RFR: JEP 359: Records (Preview) (full code)

[Vicente Romero]

Hi Jan,

I have addressed your comments on delta iteration [1]

[1] 
http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01_delta.01/ 
some comments below.

On 11/29/19 11:30 AM, Jan Lahoda wrote:
> Hi Vcente,
>
> Overall, looks fine I think. A few comments on the javac implementation:
> -in TypeEnter, I believe this:
> memberEnter.memberEnter(tree.defs.diff(List.convert(JCTree.class, 
> defsBeforeAddingNewMembers)), env);
> is unnecessary

yep removed

> (and fairly slow). defsBeforeAddingNewMembers is initialized to 
> tree.defs a few lines above, and I don't think tree.defs is modified 
> between the point defsBeforeAddingNewMembers and this line. I.e. the 
> diff will be empty, but very slow to compute (basically n^2, unless I 
> am mistaken). Not having this line would also help in not changing:
> test/langtools/tools/javac/importscope/T8193717.java
> (If the tree.defs would be changed, I would suggest to keep track of 
> which members were added, and avoid the List.diff - tree.defs can 
> contain a lot of elements, and the diff is fairly slow.)
>
> -in test/langtools/tools/javac/expswitch/ExpSwitchNestingTest.java, 
> the --enable-preview should no longer be needed

done

>
> -Flags.COMPACT_RECORD_CONSTRUCTOR says it is only for MethodSymbols, 
> but is also for VarSymbols. Either the comment should be adjusted, or 
> (possibly better), there could be a different/new flag for the fields. 
> (That new flag can reuse the same bit position as 
> COMPACT_RECORD_CONSTRUCTOR, of course.)

done

>
> -nits:
> --Lower.generateMandatedAccessors: could be rewritten using filter() 
> to avoid having an if in the forEach, or changed even more by using 
> filter-map-collect (and avoid having a manual ListBuffer). For your 
> consideration

yep done

> --AttrContext.java: no need to move the isLambda initialization, 
> correct? (i.e. the actual change is adding isSerializableLambda, but 
> the diff says isLambda is removed and added on a different place).
done
> --in 
> src/jdk.compiler/share/classes/com/sun/source/doctree/DocTree.java and 
> src/jdk.compiler/share/classes/com/sun/source/util/DocTreeFactory.java, 
> there are no changes except for a change in the copyright years - 
> these could be presumably stripped.
yep done
> --there is a TODO in:
> src/java.compiler/share/classes/javax/lang/model/SourceVersion.java
> presumably, this could be resolved based on the current spec?

I think that this comment doesn't apply as there is no need for any 
additional change. I have removed it
>
> Jan

Thanks,
Vicente

>
> On 28. 11. 19 5:37, Vicente Romero wrote:
>> Hi,
>>
>> Please review the code for the records feature at [1]. This webrev 
>> includes all: APIs, runtime, compiler, serialization, javadoc, and 
>> more! Must of the code has been reviewed but there have been some 
>> changes since reviewers saw it. Also this is the first time an 
>> integral webrev is sent out for review. Last changes on top of my 
>> mind since last review iterations:
>>
>> On the compiler implementation:
>> - it has been adapted to the last version of the language spec [2], 
>> as a reference the JVM spec is at [3]. This implied some changes in 
>> determining if a user defined constructor is the canonical or not. 
>> Now if a constructor is override-equivalent to a signature derived 
>> from the record components, then it is considered the canonical 
>> constructor. And any canonical constructor should satisfy a set of 
>> restrictions, see section 8.10.4 Record Constructor Declarations of 
>> the specification.
>> - It was also added a check to make sure that accessors are not generic.
>> - And that the canonical constructor, if user defined, is not 
>> explicitly invoking any other constructor.
>> - The list of forbidden record component names has also been updated.
>> - new error messages have been added
>>
>> APIs:
>> - there have been some API editing in java.lang.Record, 
>> java.lang.runtime.ObjectMethods and 
>> java.lang.reflect.RecordComponent, java.io.ObjectInputStream, 
>> javax.lang.model (some visitors were added)
>>
>> On the JVM implementation:
>> - some logging capabilities have been added to classFileParser.cpp to 
>> provide the reason for which the Record attribute has been ignored
>>
>> Reflection:
>> - there are several new changes to the implementation of 
>> java.lang.reflect.RecordComponent apart from the spec changes 
>> mentioned before.
>>
>> bug fixes in
>> - compiler
>> - serialization,
>> - JVM, etc
>>
>> As a reference the last iteration of the previous reviews can be 
>> found at [4] under folders: compiler, hotspot_runtime, javadoc, 
>> reflection and serialization,
>>
>> TIA,
>> Vicente
>>
>> [1] 
>> http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.00/
>> [2] 
>> http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jls.html 
>>
>> [3] 
>> http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jvms.html 
>>
>> [4] http://cr.openjdk.java.net/~vromero/records.review/
>>