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

[Maurizio Cimadamore]

Hi Srikanth,
very good piece of work. Some comments below:

* JCTree: factory product symbol - I think what you did ( stash the 
symbol in the tree) is an ok approach. If we were trying to reduce 
changes to the AST (and we do not at this stage), we could probably 
always assume that this symbol is the first local var slot of the method 
(e.g. vload_0), so no symbol is strictly needed in Gen. And, as for 
TransValues, I think we could have some visitor variable holding the 
temporary symbol. Again, not needed - just giving some suggestion in 
case you want to clean up the changes in the method AST.

* In LambdaToMethod, I believe your idea was that, for Foo::new, where 
Foo is a value, is better to generate a lambda, given that the 'new 
expression' will need to be altered by the TransValues rewriter. I think 
this is a good, foward-looking call. +1

* TreeMaker not sure whether you need the new method to create a method 
AST with a symbol in it - can't we just create the AST and then set the 
symbol? Of course I don't object much to the new factory, which is 
innocuous, it's just that it is only used in one place, and it doesn't 
seem to improve the code all that much to justify the addition.

* TransValues...

- the name 'translatingValueConstructor' seems a bit off - I would 
probably call it "insideValueConstructor". Also, this predicate kind of 
relies on the 'currentMethod' field to be set - I wonder if having a 
true predicate on the method symbol wouldn't be better and more reusable.

- I don't fully get the dance around JCNewClass when you want to create 
a default value. I think a JCNewClass tree is too heavy for a default 
value creation. What you need is an AST node with argument expression 
and a type (or a symbol, if you want). You don't need constructor 
symbols, I think - I get that this code has probably changed over time 
from a world with slightly different invariants, but looking at it now 
feels overly complex.

- on whether you need a return or not, is the problem that you don't 
know whether the code is alive or not after the last statement of the 
constructor has been evaluated? If that's the case, you *could* in 
principle feed the new method to Flow.aliveAnalyzer, and see whether it 
completes normally or not. That's the same logic we also use for lambda 
checking. E.g. you could create a new AliveAnalyzer visitor, run it on 
the body you have, and then check whether its 'alive' parameter is true 
or not. That will allow you to generate the body in full, w/o splitting 
logic between TransValues and Gen (which is of course fine as an interim 
workaround)

- shouldn't we add the new factory to the scope of the class? How is 
even ClassWriter picking it up if it's not in the scope? Ah, I see, you 
add it into the scope in getValueFactory, and you do it only once 
because of the init2factory map. I suggest to move the scope entering 
near to where the factory method is created so that, longer term, we can 
be guarateed that we won't attempt to add it to the same scope twice.

- replacing old constructor body with super call is odd. Maybe we should 
throw, to make sure nobody is calling that?

- don't you need also to override visitApply in case the constructor is 
calling  an instance method on the value being constructed?

* 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.


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