Review request:Updated JDK-8004728 Implement core reflection API for parameter reflection

[Peter Levart]

Hi Eric,

in Executable.java:

  292     private Parameter[] privateGetParameters() {
  293         if (null != parameters)
  294             return parameters.get();


If/when SoftReference is cleared,privateGetParameters  will be returning null forever. Also Executable objects are already SoftReferenced from the Class. Do we need another level of SoftReferencing?


  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     }

together with the copy constructor in Parameter.java:

   48     Parameter(Parameter p) {
   49         this.name = p.name;
   50         this.modifiers = p.modifiers;
   51         this.executable = p.executable;
   52         this.index = p.index;
   53     }

If I see right, then Parameter is an immutable object. You need not copy 
the Parameter objects when copying the array. Just do a shallow copy 
with Arrays.copy.

I assume native Executable.getParameters0() is constructing Parameter 
objects with a reference to "this" Executable. Since one already has 
access to "this" Executable (which is mutable), The Parameter objects 
that hold a back reference are not exposing any shared mutable state. 
And if they were, you would have to make a copy of Executable in the 
copy-constructor of the Parameter - not just reference the original one.


I think there is some unnecessary copying and caching of annotation 
arrays going on in Parameter.java.

You could base the annotations code on a cache that is already 
established in the package-private 
Executable.sharedGetParameterAnnotations() which returns a shared array 
of arrays of parameter annotations.


Also why do you cache entire arrays for Parameter.getType() and 
Parameter.getParameterizedType() when only a single element is needed? 
Besides, there's no need to cache anything since there's already a cache 
of that on the Executable level. You just need to expose it via 
package-private abstract methods on Executable implemented in 
Method/Constructor.


Regards, Peter


On 12/19/2012 11:43 PM, Eric McCorkle wrote:
> Hello,
>
> Please review the implementation of the core reflection API for method
> parameter reflection.  This changeset introduces a new class, Parameter,
> which represents information about method parameters.  It introduces a
> new method into Executable which returns an array of the parameters for
> the method or constructor represented by the Executable.
>
> This is part of a larger set of changes which implement portions of the
> method parameter reflection functionality in hotspot and javac.
>
>
> The open webrev is here:
> http://cr.openjdk.java.net/~jgish/~emccorkl/webrev/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