JDK 8 RFR, redux, for JDK-8005294 : Consider default methods for additions to AnnotatedElement

[Joe Darcy]

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.darcy at 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 :-(

Thanks,

-Joe