Review request:Updated JDK-8004728 Implement core reflection API for parameter reflection (original) (raw)

Peter Levart peter.levart at gmail.com
Wed Dec 19 23:47:40 UTC 2012

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/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