search

wala / WALA

T.J. Watson Libraries for Analysis, with frontends for Java, Android, and JavaScript, and may common static program analyses
http://github.com/wala/WALA
Eclipse Public License 2.0
762 stars 223 forks source link

Refactor `EclipseSourceFileModule` to make it more extension-friendly #1382

Closed khatchad closed 7 months ago

khatchad commented 7 months ago

The removed null checks seem to happen further up the class hierarchy. Replace the static construction method with a public constructor. A lone private constructor basically makes it basically impossible to subclass.

github-actions[bot] commented 7 months ago

Test Results

  572 files  ±0    572 suites  ±0   3h 41m 12s :stopwatch: +12s   734 tests ±0    717 :white_check_mark: ±0  17 :zzz: ±0  0 :x: ±0  3 554 runs  ±0  3 467 :white_check_mark: ±0  87 :zzz: ±0  0 :x: ±0 

Results for commit 1f791e30. ± Comparison against base commit 3a2ca2e5.

:recycle: This comment has been updated with latest results.

khatchad commented 7 months ago

I understand making the constructor public, but not the removal of the static method and the null checks. Why do we need the latter?

I believe that the reason the ctor was originally made private was so that the null check can be enforced. The exception throw couldn't be done in the ctor because super() needs to be the first statement. So, we have the static method invoking a private constructor.

If we make the ctor public, then you don't need the static method because you can simply just call the public ctor. So, I attempted to move the null checks to the ctor. I think this can be done as the null checking actually happens already further up the class hierarchy.

We could keep both the (now public) ctor and the static method, but I don't think anyone would use the latter if the former is available.

khatchad commented 7 months ago

I understand making the constructor public, but not the removal of the static method and the null checks. Why do we need the latter?

I believe that the reason the ctor was originally made private was so that the null check can be enforced. The exception throw couldn't be done in the ctor because super() needs to be the first statement. So, we have the static method invoking a private constructor.

If we make the ctor public, then you don't need the static method because you can simply just call the public ctor. So, I attempted to move the null checks to the ctor. I think this can be done as the null checking actually happens already further up the class hierarchy.

We could keep both the (now public) ctor and the static method, but I don't think anyone would use the latter if the former is available.

Perhaps a different way to put it is that it looked like the static method is encapsulating the object construction. But that encapsulation is too strong if we want to subclass.