RFR: 8325805: Compiler Implementation for Flexible Constructor Bodies (Second Preview) [v5]

[Archie Cobbs]

On Wed, 8 May 2024 18:19:58 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> @lahodaj,
>> 
>> Thanks for your review.
>> 
>>> Can sym be something else then a variable here (given `name` can only be `this` or `super`)?
>> 
>> It's true that `name` can only be `this` or `super` - I've added an assertion to that effect in 887699b4085.
>> 
>> It's probably also true that `sym` has to be a `VarSymbol` but that was not immediately provable just from looking at the code. I've been burned before by making such assumptions (e.g., when things like `ResolveError` can sometimes pop up) so I'm playing it safe here.
>
> I may be too strict, but: I don't see a realistic way how it could be some kind of an error symbol here (as this fetches the symbol from a `Scope`). I may be wrong. But unless there's a known/realistic way how the symbol could be something else than a VarSymbol, I think I would prefer the code to fail one way or another here, rather than continuing. Because when the execution will continue with in an unpredicted situation, we don't know what will happen.
> 
> But, as I say, it may be too strict.

OK, I've removed the (hopefully redundant) check in 619ab2b20be. I tried various erroneous inputs but could not trigger a compiler crash so it seems like you are right.

>> I agree in principle, however the code base uses `hasTag()` instead of `instanceof` everywhere - presumably for a good (although possibly obsolete) reason - so I'm just being conservative/consistent.
>> 
>> As a separate discussion, it's worth considering getting rid of 'tags' in a giant wholesale cleanup... maybe someone could write a clever script to make the changes automatically. But first someone would need to confirm that what originally motivated this quirk no longer applies.
>> 
>> Personally I've always found that `hasTag()` instead of `instanceof` just makes the code harder to read. These days I'd guess it's probably not saving any time either.
>
> We are already using pattern matching `instanceof` for trees, like here:
> https://github.com/openjdk/jdk/blob/ad78b7fa67ba30cab2e8f496e4c765be15deeca6/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java#L1215
> https://github.com/openjdk/jdk/blob/ad78b7fa67ba30cab2e8f496e4c765be15deeca6/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java#L1517
> 
> While I am not a bit fan of doing whole code-base rewrite to introduce pattern matching (even though technically we could do that), mostly because that obfuscates the history, and makes backports more difficult, I don't see a big reason to not use the feature for new code.

OK you've convince me... fixed in 0829abf466c.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18088#discussion_r1594491658
PR Review Comment: https://git.openjdk.org/jdk/pull/18088#discussion_r1594491560