Hi Peter,
On 10/28/2013 07:24 AM, Peter Levart wrote:
On 10/28/2013 06:59 AM, Joe Darcy wrote:
Hello Joel and Peter,
Working in previous feedback, please review the revised code changes for
JDK-8005294 : Consider default methods for additions to
AnnotatedElement
http://cr.openjdk.java.net/~darcy/8005294.4/
I think the versions of getAnnotationsByType and
getDeclaredAnnotation in this patch are solid, but
getDeclaredAnnotationsByType may require some more refinement. (The
asserts in getDeclaredAnnotationsByType are commented out since I was
getting a bad build as a result; I'm following up with the HotSpot
team.)
Hi Joe,
After un-comenting asserts, the one in line 404...
if (indirectlyPresent == null || indirectlyPresent.length
== 0) {
...
} else {
// assert indirectlyPresent != null
&& indirectlyPresent.length > 0;
if (resultSize == indirectlyPresent.length){
404: // assert *resultSize == 0
||* directlyPresent == null;
...should be changed to: "assert directlyPresent == null" since
resultSize (== indirectlyPresent.length) is always > 0 in this line...
Right; the old assert was a hold-over from a previous version of the code.
Then perhaps a small optimization in loop at line 416 to avoid calling
annotationType() (a Proxy method) twice:
416: for (Annotation a : getDeclaredAnnotations()) {
*Class<? extends Annotation> aType = a.annotationType();*
if (*aType*.equals(annotationClass)) {
indirectOffset = 1;
break;
} else if (*aType*.equals(containerType)) {
break;
}
}
Good suggestion.
The test looks fine. I just wonder whether you intended to uncomment
or to remove the following lines of AnnotatedElementDelegate:
228 // // FIXME -- adjust once build / vm problems resolved
229 // @Override
230 // public <T extends Annotation> T[]
getDeclaredAnnotationsByType(Class<T> annotationClass) {
231 // return base.getDeclaredAnnotationsByType(annotationClass);
232 // }
...since by un-commenting you would defeat the purpose of testing the
default method.
They should be deleted; I had that code live before I determined the
asserts were causing the build woes.
Maybe also add the following two annotations:
@AssociatedDirectOnSuperclassIndirectOnSubclass
@AssociatedIndirectOnSuperclassDirectOnSubclass
Done.
Your comments, along with some spec refinements from the other OpenJDK
list, are reflected in the next iteration of the webrev:
http://cr.openjdk.java.net/~darcy/8005294.5/
Thanks,
-Joe
