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

[Joe Darcy]

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