[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-compiler-dev
Subject:    Re: RFR: JDK-8242478: compiler implementation for records (Second Preview)
From:       Vicente Romero <vicente.romero () oracle ! com>
Date:       2020-05-05 20:13:45
Message-ID: 18921035-55ae-60f9-a4d6-bc5ad3536154 () oracle ! com
[Download RAW message or body]

thanks for the review, I can add one for interfaces and one for records, 
although records are pretty well covered there is no golden file one for 
them. We have: enums covered and annotations too with:

test/langtools/tools/javac/IllegalAnnotation.java which declares an 
annotation in a local context, the instance initializer,

I will add those two (interfaces and records) to the records-2 branch to 
push them once we integrate the feature,

Vicente


On 5/5/20 11:08 AM, Maurizio Cimadamore wrote:
> Looks good - a minor nit is that we have a golden file test for 
> checking that enums in methods are not allowed, but we don't for other 
> static constructs.
>
> Maurizio
>
> On 03/05/2020 21:57, Vicente Romero wrote:
>> Hi Maurizio,
>>
>> Thanks for the review. I have published 3 webrevs [1] is the same as 
>> the previous review. I just added it again to avoid confusion on what 
>> the other delta reviews were based on. [2] is implementing a recent 
>> change in the records spec which forbids assignments to record fields 
>> inside of the compact constructor. [3] is the patch addressing your 
>> review comments. The application order is [1, 2, 3]. The latest spec 
>> is at [4]. There are also some comments inlined below,
>>
>> On 5/1/20 8:23 AM, Maurizio Cimadamore wrote:
>>> Hi Vicente,
>>> good work - some comments below:
>>>
>>> * I think Tagir is right - consider code like this:
>>>
>>> void m() {
>>>    enum Foo { A, B };
>>> }
>>>
>>> surely we want this to fail if you compile it w/o --enable-preview ?
>>
>> right, I just read Tagir's mail too fast :|, good catch @Tagir!
>>
>>>
>>> * Check.java
>>>
>>> "if (implicitlyStatic || (flags & STATIC) != 0) {"
>>>
>>> Don't you already have the "staticOrImplicitlyStatic" var for this?
>>
>> right done
>>
>>>
>>> * Symbol.java
>>>
>>> Note that javac already has the capability of tracking if a type is 
>>> a varargs type - see this code in Type.java
>>>
>>> public ArrayType makeVarargs() {
>>>             return new ArrayType(elemtype, tsym, metadata) {
>>>                 @Override
>>>                 public boolean isVarargs() {
>>>                     return true;
>>>                 }
>>>             };
>>>         }
>>>
>>> In other words, from the element type, if you call 'makeVarargs' you 
>>> will get a special ArrayType whose isVarargs() will return true. I 
>>> think that could be used to drop the new boolean flag and make the 
>>> code generally tighter.
>>
>> I have removed the field and relied on invoking the isVarargs method 
>> on the type.
>>
>>>
>>> * Attr.java
>>>
>>> We already have a compiler diagnostic for when you try to override a 
>>> method with weaker access:
>>>
>>> # 0: message segment, 1: set of flag or string
>>> compiler.err.override.weaker.access=\
>>>     {0}\n\
>>>     attempting to assign weaker access privileges; was {1}
>>>
>>> I think the wording for the new diagnostic for when you use stronger 
>>> access could be borrowed from this one? (e.g. take the above 
>>> diagnostic and replace weaker with stronger?)
>>
>> sounds good, done
>>
>>>
>>> * LocalStaticDeclaration
>>>
>>> Since you are there, you might want to also add in the template some 
>>> missing cases:
>>>
>>> * instance initializer
>>> * constructor
>>> * static method
>>>
>>> I know some of the underlying code is shared - but better be safe 
>>> than sorry.
>>
>> yep added.
>>
>>>
>>>
>>> Cheers
>>> Maurizio
>>>
>>>
>> Thanks,
>> Vicente
>>
>> [1] http://cr.openjdk.java.net/~vromero/8242478/webrev.01/
>> [2] 
>> http://cr.openjdk.java.net/~vromero/8242478/webrev.01.forbid.field.assigment/
>> [3] 
>> http://cr.openjdk.java.net/~vromero/8242478/webrev.01.review.iteration/
>> [4] 
>> http://cr.openjdk.java.net/~gbierman/records2/20200501/specs/records-jls.html
>>> On 30/04/2020 16:10, Vicente Romero wrote:
>>>> Hi all,
>>>>
>>>> Please review the patch for the second preview of records at [1], 
>>>> the bug entry is at [2] the related spec can be found at [3], the 
>>>> JEP is at [4]. This patch implements the following changes:
>>>>
>>>> - local enums and interfaces, along with local records
>>>> - removed possibility of final modifier for record components
>>>> - removed requirement that canonical constructor must be public. 
>>>> Any access modifier must provide at least as much access as the 
>>>> record class. If a canonical constructor is implicitly declared, 
>>>> then its access modifier is the same as the record class
>>>> - check that there is a iff relation between a varargs record 
>>>> component and the corresponding parameter in the canonical 
>>>> constructor so:
>>>>
>>>>    record R(int... i) { R(int... i) {...}} is allowed but:
>>>>    record R(int... i) { R(int[] i) {...}} is not
>>>>
>>>> - new case for using @Override annotation to declare that a method 
>>>> is an accessor method for a record component
>>>>
>>>> These are the main changes, plus:
>>>> - there is one liner change to java.io.ObjectStreamClass, this is 
>>>> related to the fact that now the canonical record don't necessarily 
>>>> has to be public thus the use of Class::getDeclaredConstructor
>>>> - there is a one liner change concerning the flags of the generated 
>>>> toString method, this is a bug fix, toString didn't have the same 
>>>> flags as hashCode and equals
>>>> - I have added several tests to enforce other rules in the spec 
>>>> like the one saying that in a compact constructor fields have to be 
>>>> initialized in the same order in which the corresponding record 
>>>> component has been declared, plus some additional tests
>>>> - test RecordCompilationTests is executed twice now, first in a 
>>>> mode that is equivalent to the original test and then using an 
>>>> "empty" annotation processor. This is because some regressions have 
>>>> been discovered in the past when an annotation processor is 
>>>> present. I have also made some changes to the libraries this test 
>>>> uses.
>>>>
>>>> Thanks,
>>>> Vicente
>>>>
>>>> [1] http://cr.openjdk.java.net/~vromero/8242478/webrev.00
>>>> [2] https://bugs.openjdk.java.net/browse/JDK-8242478
>>>> [3] 
>>>> http://cr.openjdk.java.net/~gbierman/records2/20200428/specs/records-jls.html
>>>> [4] https://bugs.openjdk.java.net/browse/JDK-8242303
>>

[prev in list] [next in list] [prev in thread] [next in thread]