Looks good - but you need diagnostic fragments for the "canonical",
"compact" diagnostic bits (no need for re-review).
Maurizio
On 30/11/2019 18:29, Vicente Romero wrote:
Hi,
I have created another iteration at [1], this is just a delta from
last iteration [2]. Some comments below
[1]
http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01_delta.01/
[2] http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01
On 11/28/19 1:35 PM, Maurizio Cimadamore wrote:
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"
yes I have modified the code so that the error messages can show
"canonical" or "compact" depending on the case
* 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.
yes this is a bug, we are considering updating both the spec and the
compiler. I will submit another iteration as soon as this change is
reflected in both
* [optional - style] the env.info.isSerializableLambda could become
an enum { NONE, LAMBDA, SERIALIZABLE_LAMBDA } instead of two boolean
parameters
will do that later, I remember that there were some interactions
between these flags, they are not exclusive
* 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.
I have created a follow up issue:
https://bugs.openjdk.java.net/browse/JDK-8235149
* Are the changes in Tokens really needed?
no removed
* Check::checkUnique doesn't seem to use the added 'env' parameter -
changes should be reverted
yep done
* 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).
right done, I have created a method that check if a record component
name matches with a parameterless method in Object
Also, the serialization names are not needed (although I guess they
will come back at some point).
yes they will, I can remove them now but I guess we will need them
once we implement a lint warning
And, not sure what "get" and "set" are needed for?
removed
Maurizio
thanks,
Vicente
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/
