RFR: JDK-8240998: Implement javac changes for deconstruction patterns.
Maurizio Cimadamore
maurizio.cimadamore at oracle.com
Mon Mar 23 15:52:18 UTC 2020
Hi Jan,
the code looks generally good. I have some high-level questions:
* it would be useful to see what kind of diagnostics are generated - I
see some weird ones in the golden files (e.g.
"DeconstructionPatternErrors.out" refer to "compiler.misc.type.none")
* this might be a spec issue - there seems to be no support for varargs
extraction - e.g. if a record is varargs:
record Foo(int... is) { }
shouldn't I be able to do this:
if (o instanceof Foo(int i1, int i2, int i3)) { ... }
Some more detailed comments below:
* Attr.java - verifyCastable seems very similar to Check.checkCastable,
except for non-reifiable types - should we try to unify a bit?
* Attr.java - not sure I get the logic by which target type is
propagated or not in visitDeconstructionPattern. Seems to me that the
only case where you need propagation is when you have a nested component
of the kind "var x" - in which case the type is determined by the
expected record component type. But this:
+ boolean nestedIsValidPattern =
!nestedPatterns.head.hasTag(BINDINGPATTERN) ||
+ ((JCBindingPattern) nestedPatterns.head).vartype == null;
Seems to point to a slightly different direction - e.g. target type is
propagated even if the nested is a deconstruction pattern, but then I
don't see any use of resultInfo inside this visitor?
* parser changes - looks good :-)
* TransPattern - as usual, some comments illustrating basic desugared
shapes would be nice
Maurizio
On 19/03/2020 17:25, Jan Lahoda wrote:
> Hi,
>
> Turned out the patch had two bugs - one related to the owners of the
> temporary variables created, which then broke type annotations; and
> another that broken de-duplication. I am deeply sorry for that. An
> updated webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.01/
>
> A delta from previous round:
> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.delta.00.01/
>
> I am sorry for any inconvenience.
>
> Jan
>
> On 18. 03. 20 9:55, Jan Lahoda wrote:
>> Hi,
>>
>> I would like to ask for a review for a patch that implements the
>> deconstruction patterns, as described in JEP 375:
>> https://bugs.openjdk.java.net/browse/JDK-8235186
>>
>> The current specification draft is here:
>> http://cr.openjdk.java.net/~gbierman/jep375/jep375-20200316/specs/patterns-instanceof-jls.html
>>
>>
>> For this phase, the proposal is for javac to desugar the
>> deconstruction patterns using record accessors.
>>
>> The CSR for this change is being written here:
>> https://bugs.openjdk.java.net/browse/JDK-8240999
>>
>> The proposed patch:
>> http://cr.openjdk.java.net/~jlahoda/8240998/webrev.00
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8240998
>>
>> Any feedback is welcome!
>>
>> Thanks,
>> Jan
More information about the amber-dev
mailing list