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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Wed Jul 11 14:07:06 UTC 2018

On 11/07/18 14:35, Srikanth wrote:

On Wednesday 11 July 2018 06:38 PM, Maurizio Cimadamore wrote: Hi Srikanth, very good piece of work. Some comments below: [...]

- 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. _Agree. This is just a side effect of MakeDefault piggyacking on JCNewClass AST node with the creationMode field serving as a discriminator - NEW for references and DEFAULTVALUE for value types. We don't even need an argument expression - default value creation cannot take any parameters. Right - I missed that! Again, agree about the irrelevance of constructor symbol and type there - It is just an attempt to preserve internal consistency after having made many moons ago the decision to overload the JCNewClass tree. Sure, I was suspecting that was the case - consider everything I raised as followup-worthy - doesn't need to be addressed Right Now. I will look at simplifying the AST in a follow up ticket - though with _the canonical constructor syntax, the continued need for MakeDefault would be questionable. Well, we have internal AST node - such as LetExpr, so, it's ok to have internal nodes (WithField and Default) for value types too.

- 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) Perfect. Thanks. Will fix. - 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. Sounds good, will do. - replacing old constructor body with super call is odd. Maybe we should throw, to make sure nobody is calling that? I invested some good bit of time in the throw - but discarded it after learning that the verifier rejects the class file if there ever is a new opcode that targets a value Ok, sounds good then. - don't you need also to override visitApply in case the constructor is calling  an instance method on the value being constructed? There is some differing opinions on this - Brian says reject all uses of this (implicit or explicit) other than chained ctor calls, field read/writes. if you can read fields, then I'd expect to be able to call methods?

If we only allowed field writes (but I can understand that might be too restrictive), then I'd understand.

In any case, this seems more a case of a design discussion, outside the scope of this code review.

See point 3 in the Objectives section of https://bugs.openjdk.java.net/browse/JDK-8198749. John is of the opinion that once all fields are DA, instance method calls are OK. I am waiting for the dust to settle on this, In any case I will take this up as part of https://bugs.openjdk.java.net/browse/JDK-8205910 Sure

* 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. Will check! My plan is to fix a bunch of feedback items before push and raise a follow up ticket for others. Thanks

Maurizio

Thanks so much Maurizio, Have a restful break! 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

More information about the valhalla-dev mailing list