The changes were pushed to the records branch of the amber repo:
http://hg.openjdk.java.net/amber/amber
Harold
On 10/25/2019 9:23 AM, David Holmes wrote:
On 25/10/2019 9:49 pm, Harold Seigel wrote:
Thanks David!
I should have mentioned that I had pushed Serguei's jvmti.xml and
TestRecordAttr*.java changes earlier.
Pushed under what bug id to where? This JEP is still only a Candidate.
David
Harold
On 10/24/2019 8:40 PM, David Holmes wrote:
Hi Harold,
On 25/10/2019 3:50 am, Harold Seigel wrote:
Hi David,
Thanks for reviewing this!
Here's an updated webrev containing the below changes (and one
change requested by Serguei). Could you take a look?
http://cr.openjdk.java.net/~hseigel/records.review.rt.01/webrev/index.html
That incremental webrev looks fine.
However you didn't include Serguei's earlier change request
[Harold]>> I'll fix line 7789 in jvmti.xml before the change gets
pushed.
so just wanted to remind you on that.
Thanks,
David
Please also see in-line comments.
On 10/23/2019 7:16 PM, David Holmes wrote:
Hi Vicente, (and Harold!)
On 19/10/2019 4:44 am, Vicente Romero wrote:
Hi,
Please review the hotspot runtime and serviceability code for JEP
359 (Records).
I've looked at all the code, though not in great detail i.e I have
not validated the code changes against the proposed specification.
Support for records seems mostly "mechanical" and the patterns you
have used look appropriate. A couple of comments below.
src/hotspot/share/classfile/classFileParser.cpp
You added the check
3704 } else if (tag == vmSymbols::tag_record()) {
inside the block
3671 } else if (_major_version >= JAVA_11_VERSION) {
but I would have expected to see a new block created
} else if (_major_version >= JAVA_14_VERSION) {
to hold this code.
Done.
Style nit:
3773 const unsigned int calculated_attr_length =
parse_classfile_record_attribute(
3774 cfs,
3775 cp,
3776 record_attribute_start,
3777 CHECK);
The style in this file is align the args on the = character.
Done.
4928 if ((is_abstract && is_final && !major_gte_14) ||
As Lois mentioned already this change seems incorrect in general -
is it related to sealed types perhaps? (Even then it should be
tightened to actually check for a sealed type and not just allow
arbitrary abstract+final classes.)
The '!major_gte_14' check was initially for sealed types, but is
wrong. It's already been removed.
---
src/hotspot/share/classfile/javaClasses.cpp
+ if (ik->should_be_initialized()) {
+ ik->initialize(CHECK_0);
+ }
Unless the call to should_be_initialized is an inline method
(which it isn't) then we may as well just call initialize
unconditionally as the first thing it will do is check
should_be_initialized.
Done.
+ jio_snprintf(sig, sig_len, "()%s", type->as_C_string());
You should use the symbolic constants for the '(' and ')' characters.
Done.
---
src/hotspot/share/oops/instanceKlass.cpp
Nit:
3549 if (component) {
should test != NULL
Done.
---
test/jdk/java/lang/instrument/RedefineRecordAttr
Nice reuse of the nestmate testing pattern :)
Yes, that nestmate test was very helpful!
Thanks, Harold
---
Thanks,
David
Thanks in advance for the feedback,
Vicente
PS, Thanks to Harold for the development
[1]
http://cr.openjdk.java.net/~vromero/records.review/hotspot_runtime/webrev.00/
