Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
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.javaTue Feb 28
17:00:28 2012 +0400
+++ b/src/share/classes/java/lang/reflect/Executable.javaTue 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.
*
* pIf 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
* citeThe Javatrade; 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
* citeThe Javatrade; 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
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
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.javaTue Feb 28
17:00:28 2012 +0400
+++ b/src/share/classes/java/lang/reflect/Executable.javaTue 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.
*
* pIf 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
* citeThe Javatrade; 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
* citeThe Javatrade; 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:
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
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.javaTue Feb 28
17:00:28 2012 +0400
+++ b/src/share/classes/java/lang/reflect/Executable.javaTue 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.
*
*pIf 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
*citeThe Javatrade; 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
*citeThe Javatrade; 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
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Hi Joe,
On 29/02/2012 3:03 AM, Joe Darcy wrote:
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.
That substitution seems fine in the patch. Is there a full
webrev/blenderrev?
David
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.
*
* pIf 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
* citeThe Javatrade; 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
* citeThe Javatrade; 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
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
On 29/02/2012 10:29 AM, David Holmes wrote:
Hi Joe,
On 29/02/2012 3:03 AM, Joe Darcy wrote:
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.
That substitution seems fine in the patch. Is there a full
webrev/blenderrev?
I see it was already pushed, so I just checked the complete file.
David
David
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.
*
* pIf 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
* citeThe Javatrade; 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
* citeThe Javatrade; 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
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
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.da...@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.
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Agreed; I've posted a BlenderRev corresponding to the current patch at:
http://cr.openjdk.java.net/~darcy/7007535.4/BR-7007535.html
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.da...@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.
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
On 12:49 Tue 19 Jul , Joe Darcy wrote:
Agreed; I've posted a BlenderRev corresponding to the current patch at:
http://cr.openjdk.java.net/~darcy/7007535.4/BR-7007535.html
What is BlenderRev? Google finds others posted by Oracle employees but
not how to generate them.
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.da...@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.
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Hi Mike.
On 7/19/2011 1:54 PM, Mike Duigou wrote:
I reviewed the BlenderRev fairly closely and did not find any errors. The only weirdness
I saw was several cases where multiple Specified by: declarations were
present for a method and one of the instances referenced a class to which it didn't
appear to be able to link to.
Example: Method.getTypeParameters():
Specified by:
getTypeParameters in interface java.lang.reflect.GenericDeclaration
It wasn't clear to me why it needed two Specified by: entries and only one of
them was hot linked to the specifying class.
I did a one-off javadoc run just of the classes in question to get the
BlenderRev; I didn't include GenericDeclaration in the set of types for
which javadoc was generated, which is probably why the link was missing
for that type.
I don't know all the criteria for the generation of the Specified by:
references; however, there are other cases in the platform where more
than one appears, such as ArrayList.size:
http://download.java.net/jdk7/docs/api/java/util/ArrayList.html#size()
Thanks,
-Joe
Just javadoc weirdness?
Mike
On Jul 19 2011, at 12:49 , Joe Darcy wrote:
Agreed; I've posted a BlenderRev corresponding to the current patch at:
http://cr.openjdk.java.net/~darcy/7007535.4/BR-7007535.html
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.da...@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:
@throwsexception-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.
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
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.da...@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.
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Hi Joe,
Seems okay. Can you use blenderrev (or specdiff or some other tool) to
actually compare the generated javadoc output?
One stylistic comment. The {@inheritDoc} in the main comment block of
each method is superfluous as the default behaviour is to inherit those
javadoc attributes ie this:
/**
* {@inheritDoc}
*/
is unnecessary, and this:
/**
* {@inheritDoc}
* @throws GenericSignatureFormatError {@inheritDoc}
* @since 1.5
*/
is equivalent to:
/**
* @throws GenericSignatureFormatError {@inheritDoc}
* @since 1.5
*/
Cheers,
David
Joe Darcy said the following on 07/18/11 17:51:
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.da...@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.
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Hi Joe,
On Jul 15, 2011, at 1:49 AM, David Holmes wrote:
On 14/07/2011 12:21 PM, joe.da...@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?
In Constructor.java and Method.java, getExceptionTypes has @since 1.5, but
that method has existed in those classes since 1.1.
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?
Cheers,
-- Peter
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
On 19:21 Wed 13 Jul , joe.da...@oracle.com wrote:
Hello.
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
*/
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.
I assume this is why we have some methods in the subclass that just
call the superclass.
It would have been natural to also move common fields to Executable;
however, HotSpot treats the Constructor and Method type specially and
relies on the existing field ordering. Since altering the field layout
would require coordinated HotSpot changes, I'm opting to not perform
such a change right now. At least one abstract accessor method is
declared in Executable to still allow code sharing even though the
required field is not present. In other cases, package private instance
methods on Executable are passed the needed state from overridden public
methods in Method/Constructor.
All java/lang/reflect regression tests pass on a full build with these
changes.
The changes look good (though hard to read in places due to additional
whitespace changes mixed in). Nice to see these two finally being grouped
together.
Thanks,
-Joe
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Dr Andrew John Hughes wrote:
On 19:21 Wed 13 Jul , joe.da...@oracle.com wrote:
Hello.
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
*/
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.
I assume this is why we have some methods in the subclass that just
call the superclass.
Correct. This would not have been necessary if Executable were added
back in, say, JDK 5.
It would have been natural to also move common fields to Executable;
however, HotSpot treats the Constructor and Method type specially and
relies on the existing field ordering. Since altering the field layout
would require coordinated HotSpot changes, I'm opting to not perform
such a change right now. At least one abstract accessor method is
declared in Executable to still allow code sharing even though the
required field is not present. In other cases, package private instance
methods on Executable are passed the needed state from overridden public
methods in Method/Constructor.
All java/lang/reflect regression tests pass on a full build with these
changes.
The changes look good (though hard to read in places due to additional
whitespace changes mixed in). Nice to see these two finally being grouped
together.
Thanks,
-Joe
Re: JDK 8 code review request for 7007535: (reflect) Please generalize Constructor and Method
Hi Joe,
On 14/07/2011 12:21 PM, joe.da...@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}
Cheers,
David
-
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.
It would have been natural to also move common fields to Executable;
however, HotSpot treats the Constructor and Method type specially and relies
on the existing field ordering. Since altering the field layout would
require coordinated HotSpot changes, I'm opting to not perform such a change
right now. At least one abstract accessor method is declared in Executable
to still allow code sharing even though the required field is not present.
In other cases, package private instance methods on Executable are passed
the needed state from overridden public methods in Method/Constructor.
All java/lang/reflect regression tests pass on a full build with these changes.
Thanks,
-Joe
