Request for review: JDK-8004728 Hotspot support for parameter reflection

[John Rose]

On Jan 4, 2013, at 6:14 PM, Eric McCorkle wrote:

> Hello,
> 
> Please review the following change set, which implements support for method parameter reflection in hotspot.  The open webrev can be found here:
> 
> http://cr.openjdk.java.net/~acorn/8004728/webrev

Cool stuff!  One quick comment on this loop in the CFP:

+  // Copy method parameters
+  if (method_parameters_length > 0) {
+    MethodParametersElement* elem = m->constMethod()->method_parameters_start();
+    for(int i = 0; i < method_parameters_length; i++) {
+      elem[i].name_cp_index =
+	Bytes::get_Java_u2(method_parameters_data);
+      method_parameters_data += 2;
+      elem[i].flags = Bytes::get_Java_u4(method_parameters_data);
+      method_parameters_data += 4;
+    }
+  }

This uses a C style (or C++ STL style) bumped pointer (*elemptr++).

In the Hotspot source base is is more common, and preferable, to use the array/index style (array[index++]).

In the case of the above code, the variable method_parameters_data starts off as the address of a classfile attribute, and is then repurposed as an index variable.  It is misleading to give one name and one declaration to the two functions.  For example, if code later on were to attempt to refer to method_parameters_data again, it would get garbage (the final value of the index).  This sort of bug is a real possibility with single-name loop indexing, which the double-name idiom (array[index]) avoids cleanly.

For a model of a similar job in the CFP, see this line and related lines:
  copy_lvt_element(&cf_lvt[idx], lvt);

Note that cf_lvt is not incremented; the idx is instead.  This is the preferred style in Hotspot.

HTH
— John