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

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 
> DEFAULT_VALUE 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 <init>
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
>>
>