Thanks Lois!
Please see comments inline.
On 10/21/2019 2:40 PM, Lois Foltan wrote:
On 10/18/2019 2:44 PM, Vicente Romero wrote:
Hi,
Please review the hotspot runtime and serviceability code for JEP 359
(Records).
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/
Harold, Vicente,
Overall this looks good! Some comments:
src/hotspot/share/classfile/classFileParser.cpp
- line #3226: I mentiond this below that I would like to see the
comment from jvmtiClassFileReconstituter.cp ahead of
ClassFileParser::parse_classfile_record_attribute() so it is easy to
follow the expected layout.
Done.
- line #3275: Ahead of the for loop it would be good to add a comment
listing what the expected attributes are for Records.
Done.
- line #4732: The added check of "!major_gte_14" looks incorrect for
final abstract classes. That isn't relevant to Records, correct?
That's a bug! Thanks for finding it. I'll remove the "!major_gte_14".
src/hotspot/share/prims/jvm.cpp
- line #1737 - #1739: use oopFactory::new_objArrayHandle() instead of
breaking this accross 2 lines.
I don't think this should be done because of the CHECK_NULL:
objArrayOop record_components =
oopFactory::new_objArray(SystemDictionary::RecordComponent_klass(),
length, CHECK_NULL);
objArrayHandle components_h (THREAD, record_components);
- line #1751: please add a comment that indicates the behavior when
the InstanceKlass is not a record. It seems like an empty array is
returned?
Done.
src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp
- copyright update needed.
- line #427-438: I like the comment, can you add that ahead of
ClassFileParser::parse_classfile_record_attribute().
Done.
src/hotspot/share/prims/jvmtiRedefineClasses.cpp
- line #839 - comment "of" should be "if"?
Done.
src/hotspot/share/oops/recordComponent.hpp
- line #95 - determination of TBD comment needed before committing.
Done.
I still need to review the tests.
Thanks! Harold
Thanks,
Lois
