Review request JDK-8004729: Parameter Reflection API

[Remi Forax]

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