Review request JDK-8004729: Parameter Reflection API

[Joe Darcy]

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.

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.

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.

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

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/view_bug.do?bug_id=8004729
>
> The latest version of the spec can be found here:
> http://cr.openjdk.java.net/~abuckley/8misc.pdf
>
>
> Thanks,
> Eric