Hi Vicente,
generally looks good - few comments below; I tried to focus on areas
where the compiler code seemed to diverge from the spec, as well on
pieces of code which look a leftover from previous spec rounds.
* canonical constructors *can* have return statements - compact
constructors cant; the spec went a bit back and forth on this, but now
it has settled. Since compact constructors are turned into ordinary
canonical ones by the parser, I think you need to add an extra check for
COMPACT_RECORD_CONSTRUCTOR; in other words, this is ok:
record Foo() {
Foo() { return; } //canonical
}
this isn't
record Foo() {
Foo { return; } //compact
}
but the compiler rejects both. This probably means tweaking the
diagnostic a bit to say "a compact constructor must not contain |return|
statements"
* in general, all diagnostics speak about 'canonical constructor'
regardless of whether the constructor is canonical of compact; while I
understand the reason behind what we get now, some of the error messages
can be confusing, especially if you look at the spec, where canonical
constructor and compact constructor are two different concepts. This
should be fixed (even if not immediately, in which case I'd recommend to
file a JBS issue to track that)
* static accessors are allowed - this means that I can do this:
record Foo(int x) {
public static int x() {return 0; }
public static void main(String[] args) {
System.err.println(new Foo(42).x());
}
}
This will compile and print 0. The classfile will contain the following
members:
final class Foo extends java.lang.Record {
public Foo(int);
public static int x();
public static void main(java.lang.String[]);
public java.lang.String toString();
public final int hashCode();
public final boolean equals(java.lang.Object);
}
I believe this is an issue in the compiler, but also in the latest spec
draft, if I'm not mistaken.
* [optional - style] the env.info.isSerializableLambda could become an
enum { NONE, LAMBDA, SERIALIZABLE_LAMBDA } instead of two boolean parameters
* this code is still rejected with --enable-preview _disabled_:
class X {
record R(int i) {
return null;
}
}
class record {}
This gives the error:
Error:
| records are a preview feature and are disabled by default.
| (use --enable-preview to enable records)
| record R(int i) { return null } }
| ^
| Error:
| illegal start of type
| record R(int i) { return null } }
| ^
In other words, the parsing logic for members is too aggressive - we
shouldn't call isRecordStart() in there. If this is not fixed in this
round, we should keep track with a JBS issue.
* Are the changes in Tokens really needed?
* Check::checkUnique doesn't seem to use the added 'env' parameter -
changes should be reverted
* Names.jave - the logic for having forbiddenRecordComponentNames could
use some refresh - in the latest spec we basically have to ban
components that have same name as j.l.Object members - so I think we can
implement the check more directly (e.g. w/o having a set of names).
Also, the serialization names are not needed (although I guess they will
come back at some point). And, not sure what "get" and "set" are needed for?
Maurizio
On 28/11/2019 16:05, Vicente Romero wrote:
Hi again,
Sorry but I realized that I forgot to remove some code on the compiler
side. The code removed is small, before we were issuing an error if
some serialization methods were declared as record members. That
section was removed from the spec. I have prepared another iteration
with this change at [1]
Thanks,
Vicente
[1]
http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01/
On 11/27/19 11:37 PM, 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/
