JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Mike Duigou
mike.duigou at oracle.com
Tue Feb 28 18:52:46 UTC 2012
These changes look good to me. Is there a new CR for the javadoc changes?
Mike
On Feb 28 2012, at 09:03 , Joe Darcy wrote:
> 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