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
