RFR:JDK-8198749 Translation of value constructors in classic constructor notation (original) (raw)

Srikanth srikanth.adayapalam at oracle.com
Thu Jul 12 11:15:59 UTC 2018

Here is the push URL: http://hg.openjdk.java.net/valhalla/valhalla/rev/5d5382d7200c

See below for action taken report.

Thanks a lot for the review comments Maurizio. Appreciate it! Srikanth

On Wednesday 11 July 2018 06:38 PM, Maurizio Cimadamore wrote:

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. vload0), 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.

As documented in previous mail, I undertook this noble pursuit in earnest and it took me to some interesting corners that negated the initial promising simplification that appeared possible. So at least for now, I will stay with the present implementation. IMO it is not unreasonable that a parse tree node would store a computed/derived attribute as opposed to what was primarily present in source.

* 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

Thanks

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

I have withdrawn this change.

* 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. Name simplified to constructingValue()

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

Agreed. I have raised https://bugs.openjdk.java.net/browse/JDK-8207168.

- 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)

This again I have deferred after taking a look.

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

Done.

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

As discussed elsewhere, a throw is not done because verifier anyways rejects any attempts to reach view new. new and VT don't go together. So ATM constructors are stubbed out with just a supercall which is required by the verifier.

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

This will be covered as part of JDK-8205910

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

I think this is a non-issue, anyway I have added the following as a test FWIW.

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

More information about the valhalla-dev mailing list