RFR: JEP 359-Records: hotspot runtime and serviceability code

[Harold Seigel]

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

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/ 
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20191024/5428fffe/attachment-0001.html>