RFR:JDK-8198749 Translation of value constructors in classic constructor notation

[Srikanth]

On Wednesday 11 July 2018 06:38 PM, Maurizio Cimadamore wrote:
>
> * tests - mayeb for later, and depending on whether we support inner 
> values (seems to, looking at InnerValueNew) - one kind of super 
> constructor call that might get you in trouble is the qualified super 
> constructor call - e.g., with regular ordinary classes you can have this:
>
> class A {
>
>     class Inner { }
> }
>
>
> class B extends A.Inner {
>
>        B(A encl) {
>            enc.super();
>        }
> }
>
>
> It would be interesting to verify at some point that this kind of 
> idiom works with your value desugaring too.
>


Perhaps I am missing some nuance:

Of the three classes involved, A, B and A.Inner, only A can be a value 
class. B and A.Inner cannot be since values don't extend and can't be 
extended.

In that case, after the prevailing desugaring/lowering we end up with

class B extends A$Inner {

     B(A encl) {
         super(<*nullchk*>encl);
     }
}

that is being fed as *input* to the new translator. The nullcheck is 
bogus and should be eliminated for
value typed `encl' - but what other complication do you see ?

I do have tests that verify that constructors that receive synthetics 
such as enclosing instances and captured outer locals are translated 
properly.

Thanks
Srikanth


>
> That's all I can think of.
>
> Cheers
> Maurizio
>
>
>
> On 11/07/18 11:56, Srikanth wrote:
>>
>> Hi Maurizio,
>>
>> Please review the following patch that implements the changes 
>> required for
>> https://bugs.openjdk.java.net/browse/JDK-8198749 ([lworld] 
>> Translation of value constructors in classic constructor notation)
>>
>> Webrev: http://cr.openjdk.java.net/~sadayapalam/JDK-8198749/webrev.00/
>>
>> Part of the work in supporting value construction in the classic 
>> constructor notation
>> will happen independently on behalf of 
>> https://bugs.openjdk.java.net/browse/JDK-8205910(diagnose use of 
>> 'this' with DU fields (for VTs and VBCs)).
>>
>> Normally I would have requested the review after a fair bit more of 
>> testing and self-review, but given your proposed long absence, I am 
>> requesting the review now. However, I am happy to note that the code 
>> is in pretty decent shape for another person to study (known issues 
>> at the bottom)
>>
>> I will continue with the testing and self review in parallel.
>>
>> If time is an issue, you can limit the review to the following source 
>> files:
>>
>>     Gen.java
>>     TransValues.java
>>     LambdaToMethod.java
>>
>> and these test files: (these give an idea of what works already and 
>> allows you to experiment by tweaking)
>>
>> test/langtools/tools/javac/valhalla/lworld-values/InnerValueNew.java
>> test/langtools/tools/javac/valhalla/lworld-values/LocalValueNew.java
>> test/langtools/tools/javac/valhalla/lworld-values/QualifiedThisTest.java
>> test/langtools/tools/javac/valhalla/lworld-values/ValueConstructorRef.java 
>>
>> test/langtools/tools/javac/valhalla/lworld-values/ValueNewReadWrite.java
>>
>> Known issues:
>>
>> (1) make.at() calls may not be updated consistently/uniformly.
>> (2) I need to double check that some subtree translations are not 
>> skipped inadvertently.
>> (3) Some of the other *older* modified tests need rewriting - they 
>> compare javap output which is flaky due to constant pool offsets 
>> changing anytime there is a change in code generation scheme.
>> (4) Some code duplication can be avoided by creating utility routines.
>>
>> langtools tests are green (except for one non-material failure).
>>
>> Thanks!
>> Srikanth
>
>