On 10/30/2013 02:57 AM, Joe Darcy wrote:
On 10/29/2013 06:37 PM, Joe Darcy wrote:
Hi Joel,
On 10/29/2013 09:20 AM, Joel Borggrén-Franck wrote:
Hi Joe, Peter,
On 29 okt 2013, at 07:09, Joe Darcy <joe.da...@oracle.com> wrote:
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,
259 if (result.length == 0 && // Neither directly nor
indirectly present
260 this instanceof Class && // the element is a class
261 AnnotationType.getInstance(annotationClass).isInherited()) {
// Inheritable
262 ...
j.l.Class is final and has an implementation of getAnnotationsByType
so everything in this branch is dead code. I understand you might
want to document how the lookup is supposed to happen if you have
inheritance semantics for your AnnotatedElement but IMHO this isn't
the way to do it. Just delegate to getDeclaredAnnotationsByType.
Summarizing an off-list discussion, in terms of making this
particular method robust in the face of of possible alternative
versions of java.lang.Class and friends, I think it is acceptable to
introduce what will in practice be dead code with the version of
java.lang.Class in JDK 8.
291 * @since 1.8
292 */
293 default <T extends Annotation> T
getDeclaredAnnotation(Class<T> annotationClass) {
I keep forgetting that this one is new to 8. My reservations about
making this a default was based on (the misconception) that this was
a 5 method. This method looks good.
For getDeclaredAnnotationsByType() the idea of having two
implementations of the same logic, one taking arrays as arguments
and the other Maps, seems wrong. I would strongly prefer if we only
had one implementation, calling into AnnotationSupport from this
method. Something like
default <T extends Annotation> T[]
getDeclaredAnnotationsByType(Class<T> annotationClass) {
Objects.requireNonNull(annotationClass);
return
AnnotationSupport.getDirectlyAndIndirectlyPresent(Arrays.stream(getDeclaredAnnotations()).collect(Collectors.toMap(
(a -> a.getClass()),
Function.identity())),
annotationClass);
}
I can't see these default methods as the code we should be spending
time optimizing.
Given the improved maintenance situation and the low expecte duty
cycle of the default method implementations, despite having worked on
what should be a fast-ish getDeclaredAnnotation implementation, I'm
willing to replace it by a call to AnnotationSupport wrapped around a
call to getDeclaredAnnotations().
This style of implementation is allowed by the current @implSpec for
the method.
However, I'm not sure the
AnnotationSupport.getDirectlyAndIndirectlyPresent call can work as
currently formulated will work since I believe what needs to be
called is "a.getAnnotationType()" rather than "a.getClass()", but
unfortunately "getAnnotationType()" is not defined on
AnnotatedElement so reflection would be needed to call
getAnnotationType :-(
I retract my previous comments; I was confusing "annotationType" and
"value()" in terms of where they are defined. This version of
getDeclaredAnnotationsByType passes the battery of regression tests:
default <T extends Annotation> T[]
getDeclaredAnnotationsByType(Class<T> annotationClass) {
Objects.requireNonNull(annotationClass);
return AnnotationSupport.
getDirectlyAndIndirectlyPresent(Arrays.stream(getDeclaredAnnotations()).
collect(Collectors.toMap(
(a -> a.annotationType()),
Function.identity(),
((first,second) -> first),
() -> new LinkedHashMap<>() )),
annotationClass);
}
Hi Joe,
I suggest using method references where possible, which saves from
generating two (of three) synthetic methods in AnnotatedElement interface:
Collectors.toMap(
Annotation::annotationType,
Function.identity(),
(first, second) -> first,
LinkedHashMap::new)
Regards, Peter
I've prepared a new version of the webrev with this change as well as
a slightly-refactored regression test:
http://cr.openjdk.java.net/~darcy/8005294.6
Thanks,
-Joe
