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

[Peter Levart]

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