JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Joe Darcy
joe.darcy at oracle.com
Tue Feb 28 18:55:54 UTC 2012
On 2/28/2012 10:52 AM, Mike Duigou wrote:
> These changes look good to me. Is there a new CR for the javadoc changes?
Thanks Mike; I was planning to file the bug after the reviews came in.
-Joe
> 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