RFR: JEP 359: Records (Preview) (full code)
jan.lahoda at oracle.com
Fri Nov 29 16:30:57 UTC 2019
Overall, looks fine I think. A few comments on the javac implementation:
-in TypeEnter, I believe this:
is unnecessary (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:
(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
-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.)
--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
--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).
there are no changes except for a change in the copyright years - these
could be presumably stripped.
--there is a TODO in:
presumably, this could be resolved based on the current spec?
On 28. 11. 19 5:37, Vicente Romero wrote:
> Please review the code for the records feature at . 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
> On the compiler implementation:
> - it has been adapted to the last version of the language spec , as a
> reference the JVM spec is at . 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
> - 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
> - there are several new changes to the implementation of
> java.lang.reflect.RecordComponent apart from the spec changes mentioned
> bug fixes in
> - compiler
> - serialization,
> - JVM, etc
> As a reference the last iteration of the previous reviews can be found
> at  under folders: compiler, hotspot_runtime, javadoc, reflection and
>  http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.00/
>  http://cr.openjdk.java.net/~vromero/records.review/
More information about the compiler-dev