JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method

Joe Darcy joe.darcy at oracle.com
Tue Feb 28 17:03:24 UTC 2012


Hi David,

Belatedly catching up on email, please review the patch below to address 
the issues you've raised.  I searched for "method" and replaced it with 
"executable" as appropriate throughout the javadoc of the class.

Thanks,

-Joe

--- a/src/share/classes/java/lang/reflect/Executable.java    Tue Feb 28 
17:00:28 2012 +0400
+++ b/src/share/classes/java/lang/reflect/Executable.java    Tue Feb 28 
09:01:27 2012 -0800
@@ -180,7 +180,7 @@

      /**
       * Returns the {@code Class} object representing the class or 
interface
-     * that declares the method represented by this executable object.
+     * that declares the executable represented by this object.
       */
      public abstract Class<?> getDeclaringClass();

@@ -215,18 +215,18 @@
       * Returns an array of {@code Class} objects that represent the formal
       * parameter types, in declaration order, of the executable
       * represented by this object.  Returns an array of length
-     * 0 if the underlying method takes no parameters.
+     * 0 if the underlying executable takes no parameters.
       *
-     * @return the parameter types for the method this object
+     * @return the parameter types for the executable this object
       * represents
       */
      public abstract Class<?>[] getParameterTypes();

      /**
       * Returns an array of {@code Type} objects that represent the formal
-     * parameter types, in declaration order, of the method represented by
-     * this executable object. Returns an array of length 0 if the
-     * underlying method takes no parameters.
+     * parameter types, in declaration order, of the executable 
represented by
+     * this object. Returns an array of length 0 if the
+     * underlying executable takes no parameters.
       *
       * <p>If a formal parameter type is a parameterized type,
       * the {@code Type} object returned for it must accurately reflect
@@ -236,16 +236,16 @@
       * type, it is created. Otherwise, it is resolved.
       *
       * @return an array of {@code Type}s that represent the formal
-     *     parameter types of the underlying method, in declaration order
+     *     parameter types of the underlying executable, in declaration 
order
       * @throws GenericSignatureFormatError
       *     if the generic method signature does not conform to the format
       *     specified in
       * <cite>The Java™ Virtual Machine Specification</cite>
       * @throws TypeNotPresentException if any of the parameter
-     *     types of the underlying method refers to a non-existent type
+     *     types of the underlying executable refers to a non-existent type
       *     declaration
       * @throws MalformedParameterizedTypeException if any of
-     *     the underlying method's parameter types refer to a parameterized
+     *     the underlying executable's parameter types refer to a 
parameterized
       *     type that cannot be instantiated for any reason
       */
      public Type[] getGenericParameterTypes() {
@@ -277,15 +277,15 @@
       * type, it is created. Otherwise, it is resolved.
       *
       * @return an array of Types that represent the exception types
-     *     thrown by the underlying method
+     *     thrown by the underlying executable
       * @throws GenericSignatureFormatError
       *     if the generic method signature does not conform to the format
       *     specified in
       * <cite>The Java™ Virtual Machine Specification</cite>
-     * @throws TypeNotPresentException if the underlying method's
+     * @throws TypeNotPresentException if the underlying executable's
       *     {@code throws} clause refers to a non-existent type declaration
       * @throws MalformedParameterizedTypeException if
-     *     the underlying method's {@code throws} clause refers to a
+     *     the underlying executable's {@code throws} clause refers to a
       *     parameterized type that cannot be instantiated for any reason
       */
      public Type[] getGenericExceptionTypes() {
@@ -330,7 +330,7 @@
       * Returns an array of arrays that represent the annotations on
       * the formal parameters, in declaration order, of the executable
       * represented by this object. (Returns an array of length zero if
-     * the underlying method is parameterless.  If the executable has
+     * the underlying executable is parameterless.  If the executable has
       * one or more parameters, a nested array of length zero is
       * returned for each parameter with no annotations.) The
       * annotation objects contained in the returned arrays are


On 07/20/2011 01:03 AM, David Holmes wrote:
> Just realized this has come in too late ...
>
> Joe Darcy said the following on 07/20/11 05:49:
>> Agreed; I've posted a BlenderRev corresponding to the current patch at:
>>
>>    http://cr.openjdk.java.net/~darcy/7007535.4/BR-7007535.html
>
> Thanks. So now I can more readily see that the doc for Executable 
> isn't quite suitable for Constructor in a few places:
>
> getDeclaringClass:
>
> 183     /**
> 184      * Returns the {@code Class} object representing the class or 
> interface
> 185      * that declares the method represented by this executable 
> object.
> 186      */
>
> For Constructor "method" should be "constructor". But I think, looking 
> at the terminology elsewhere that the above could be rewritten as:
>
> "Returns the Class object representing the class or interface that 
> declares the executable represented by this object."
>
> getParameterTypes:
>
> 219      * represented by this object.  Returns an array of length
> 220      * 0 if the underlying method takes no parameters.
>
> method -> executable
>
> getGenericParameterTypes:
>
> 229      * parameter types, in declaration order, of the method 
> represented by
> 230      * this executable object. Returns an array of length 0 if the
> 231      * underlying method takes no parameters.
>
> Again change to " the executable represented by this object".
> And then: underlying method -> underlying executable
>
> 241      *     parameter types of the underlying method, in 
> declaration order
> 243      *     if the generic method signature does not conform to the 
> format
> 247      *     types of the underlying method refers to a non-existent 
> type
> 250      *     the underlying method's parameter types refer to a 
> parameterized
>
> method -> executable
>
> In fact do a search for "method" in Executable and most occurrences 
> should be changed to "executable".
>
>
> Finally, in getModifiers, why delete the "as an integer. The Modifier 
> class should be used to decode the modifiers. " ?
>
>
> BTW I also find the multiple "Specified by:" references to be quite 
> strange. There can only be one specification for a method.
>
> David
> -----
>
>> Thanks,
>>
>> -Joe
>>
>> Mike Duigou wrote:
>>> This looks good to me but I agree with David that it's probably 
>>> important to look at the generated javadoc and specdiff output 
>>> before finalizing.
>>>
>>> Mike
>>>
>>> On Jul 18 2011, at 00:51 , Joe Darcy wrote:
>>>
>>>
>>>> Peter and David.
>>>>
>>>> Thanks for the careful review; the @throws information still needs 
>>>> its own {@inheritDoc}; I've uploaded a webrev with this and other 
>>>> corrections:
>>>>
>>>> http://cr.openjdk.java.net/~darcy/7007535.4
>>>>
>>>> More comments interspersed below.
>>>>
>>>> Thanks,
>>>>
>>>> -Joe
>>>>
>>>> Peter Jones wrote:
>>>>> Hi Joe,
>>>>>
>>>>> On Jul 15, 2011, at 1:49 AM, David Holmes wrote:
>>>>>
>>>>>> On 14/07/2011 12:21 PM, joe.darcy at oracle.com wrote:
>>>>>>> Please code review my JDK 8 changes for
>>>>>>>
>>>>>>> 7007535: (reflect) Please generalize Constructor and Method
>>>>>>> http://cr.openjdk.java.net/~darcy/7007535.3
>>>>>>>
>>>>>>> To summarize the changes, a new superclass is defined to capture 
>>>>>>> the common
>>>>>>> functionality of java.lang.reflect.Method and 
>>>>>>> java.lang.reflect.Constructor.
>>>>>>> That superclass is named "Executable" along the lines of
>>>>>>> javax.lang.model.ExecutableElement, which models constructors 
>>>>>>> and methods in
>>>>>>> the JSR 269 language model.
>>>>>>>
>>>>>>> Both specification and implementation code are shared. To 
>>>>>>> preserve the right
>>>>>>> @since behavior, it is common that in Method/Constructor the 
>>>>>>> javadoc for a
>>>>>>> method will now look like:
>>>>>>>
>>>>>>> /**
>>>>>>> * {@inheritDoc}
>>>>>>> * @since 1.5
>>>>>>> */
>>>>>> Unless they have fixed/changed javadoc (entirely possible) it 
>>>>>> used to be that the above would not cause @throws declarations 
>>>>>> for unchecked exceptions to be inherited - you have/had to 
>>>>>> explicitly repeat them as:
>>>>>>
>>>>>> @throws <exception-type> {@inheritDoc}
>>>>> Yes, that would seem to be needed for some of the inherited 
>>>>> getters of generics info, which specify unchecked exception types.
>>>>>
>>>>>
>>>>>>> Since Executable is being created in JDK 8, it would be 
>>>>>>> incorrect for
>>>>>>> methods in that class to have an @since of 1.5; adding the 
>>>>>>> @since in
>>>>>>> Method/Constructor preserves the right information.
>>>>> In Executable.java, getAnnotation and getDeclaredAnnotations do 
>>>>> have "@since 1.5"-- oversight?
>>>>>
>>>> Yes; that was incorrect.
>>>>
>>>>> In Constructor.java and Method.java, getExceptionTypes has "@since 
>>>>> 1.5", but that method has existed in those classes since 1.1.
>>>>>
>>>>>
>>>> Fixed.
>>>>
>>>>> In Executable.java:
>>>>>
>>>>> 216     /**
>>>>> 217      * Returns an array of {@code Class} objects that 
>>>>> represent the formal
>>>>> 218      * parameter types, in declaration order, of the method
>>>>> 219      * represented by this {@code Method} object.  Returns an 
>>>>> array of length
>>>>> 220      * 0 if the underlying method takes no parameters.
>>>>> 221      *
>>>>> 222      * @return the parameter types for the method this object
>>>>> 223      * represents
>>>>>
>>>>> At least "{@code Method}" needs to be generalized, and perhaps all 
>>>>> occurrences of "method"?
>>>>>
>>>> Corrected.
>>>
>>




More information about the core-libs-dev mailing list