Review request JDK-8004729: Parameter Reflection API (original) (raw)

Remi Forax forax at univ-mlv.fr
Thu Jan 10 08:45:10 UTC 2013

On 01/10/2013 07:23 AM, Joe Darcy wrote:

Hi Eric,

A few quick comments. In Executable, 228 /** 229 * Returns the number of formal parameters for the executable 230 * represented by this object. 231 * 232 * @return The number of formal parameters for the executable this 233 * object represents 234 */ 235 public abstract int getNumParameters(); I think it is important of the getNumParameters javadoc is clarified to state "... represented by this object, including any synthesized and synthetic parameters." In other words, the number of parameters of the VM view of the query and not (necessarily) the language view.

And please rename getNumParameters to getParameterCount, so all methods related to parameters starts with the same prefix.

Also in Executable, I think the body of 279 public Parameter[] getParameters() { 280 // TODO: This may eventually need to be guarded by security 281 // mechanisms similar to those in Field, Method, etc. 282 Parameter[] raw = privateGetParameters(); 283 Parameter[] out = new Parameter[raw.length]; 284 // Need to copy the cached array to prevent users from messing 285 // with it 286 for (int i = 0; i < raw.length; i++) { 287 out[i] = new Parameter(raw[i]); 288 } 289 return out; 290 } could be replaced with return privateGetParameters().clone(); IIRC, other parts of core reflection have similar calls to clone.

yes, and the copy constructor in Parameter is not needed (we are in Java after all, not in C++). Also null == foo is not a standard idiom in Java because there is no operator overloading (likewise i++ doesn't need to be replaced by ++i).

I would prefer to see the getNumParameters method in Executable be declared to be non-abstract, but with a body that threw some kind of exception, even an abstract-method-error. Since only types within java.lang.reflect can subclass Executable, it is not generally extensible. Alternate implementations of java.lang.reflect should not be forced to not share a getNumParameters implementation from Executable.

yes !

All the public methods and constructors in java.lang.reflect.Parameter need proper javadoc. I strongly recommend rewriting the tests in the style used, for example, here: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0a1398021c7c where an annotation is used to record the expect result and then a simple checker loops over the reflective constructs and verifies that the computed values match the expected ones. The tests should include cases where the VM has a different notion of the number of parameter than the language; typical cases are constructors of inner classes (compiler is required to insert a leading other$this parameter) and constructors of enums (javac prepends two synthesized parameters). Cheers, -Joe

cheers, Rémi

On 01/09/2013 01:55 PM, Eric McCorkle wrote: Hello,

Please review the core reflection API implementation of parameter reflection. This is the final component of method parameter reflection. This was posted for review before, then delayed until the check-in for JDK-8004728 (hotspot support for parameter reflection), which occurred yesterday. Note: The check-in of JDK-8004728 was into hsx/hotspot-rt, not jdk8/tl; therefore, it may be a while before the changeset makes its way into jdk8/tl. Also note: since the check-in of JDK-8004727 (javac support for parameter reflection), there has been a failure in the tests for Pack200. This is being addressed in a fix contributed by Kumar, which I believe has also been posted for review. The open webrev is here: http://cr.openjdk.java.net/~coleenp/JDK-8004729 The feature request is here: http://bugs.sun.com/viewbug.do?bugid=8004729 The latest version of the spec can be found here: http://cr.openjdk.java.net/~abuckley/8misc.pdf

Thanks, Eric

More information about the core-libs-dev mailing list