[amber-record] feedback on a Record class attribute implementation

[David Holmes]

Hi Vicente,

On 31/10/2018 10:59 AM, Vicente Romero wrote:
> Hi all,
> 
> I have sent the patch to the hotspot list as this patch is touching some 
> native code and I would like to have some feedback from the runtime 
> wizards, please let me know if there is a better list for that. This 

hotspot-runtime-dev :)

A couple of minor comments:

src/hotspot/share/classfile/classFileParser.cpp

+  // Set nest members attribute to default sentinel
+  _record_params = Universe::the_empty_short_array();

Comment needs updating.

-        } else {
-          // Unknown attribute
-          cfs->skip_u1(attribute_length, CHECK);
+        } else if (tag == vmSymbols::tag_record()) {
...

You need to insert the new check before the existing else clause but 
need to keep the check for an unknown attribute in the given classfile 
version. Presumably this will eventually be processed only for some 
future classfile version.

---

src/hotspot/share/oops/instanceKlass.hpp

+  u2              _record_params_count;

Do you need a separate count when you can query the array length?

Cheers,
David

> patch has been pushed the the record branch in amber project [1]. The 
> records project is about, well adding data classes to the language so 
> that this declaration:
> 
> record Record(int i);
> 
> gets lowered to:
> 
> class Record {
>      final int i;
> 
>     // automatically generated equals, getters, hashCode, toString and more
> }
> 
> apart from the usual information generated for the lowered version, the 
> javac compiler is generating this new attribute in the class file:
> 
>          Record {
>              u2 name_index;
>              u4 length;
>              u2 num_record_params;
>              {
>                  u2 param_name_idx;  // [1]
>                  u2 param_flag;
>                  u2 param_desc;
>                  u2 param_signature;
>              } record_params[num_record_params];
>          }
> 
> which have a lot in common with the fields information but we don't want 
> to depend on the order of the fields etc. The attached patch provides 
> for parsing this attribute, plus additional helper classes, plus all the 
> pipes needed. As a way to provide a way for users to peek the 
> information contained in the Record attribute, I have added a method to 
> java.lang.Class, Class::getRecordParameters. In the background I'm using 
> JNI to extract the information from the related InstanceKlass in the 
> native world. Method java.lang.Class::getRecordParameters just returns 
> an array of fields but only those that have being defined in the header 
> of the record. For example if the record would have been defined as:
> 
> record Record(int i) {
>      static final int j = 0;  //no instance fields can be defined in 
> records
> }
> 
> then an invocation of java.lang.Class::getRecordParameters will return 
> only field `i` ignoring the static field `j`
> 
> TIA,
> Vicente
> 
> [1] http://hg.openjdk.java.net/amber/amber