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/
