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...
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;
}
}
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.
Maybe also add the following two annotations:
@AssociatedDirectOnSuperclassIndirectOnSubclass
@AssociatedIndirectOnSuperclassDirectOnSubclass
Regards, Peter
Also new in this version, the test program fleshed out.
Thank,
-Joe
