RFR 7122142 : (ann) Race condition between isAnnotationPresent and getAnnotations

[Peter Levart]

On 07/15/2013 11:04 AM, Aleksey Shipilev wrote:
> Hi, Peter,
>
> Not a reviewer, and have barely any time to have the careful review, so
> just a few nitpicks below:
>
> On 07/09/2013 12:54 AM, Peter Levart wrote:
>> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationType/webrev.05/
> AnnotationParser.java:
>   - Do you want make contains() generic?
>        static <T> boolean contains(T[] array, T element);

Hi Aleksey,

I was guided by the Collection.contains() signature:

Collection<T> {
     boolean contains(Object o);

Now I could do this:

static <T> boolean contains(T[] array, Object element);

But 'T' then looses it's purpose. I think the method is good as is. It's 
type-safe for any input.

>   - Also, you might probably put the null-check in contains(), and
> simplify the usage

I know it's a matter of style mostly, but when I see the following code:

         if (selectAnnotationClasses != null &&
             !contains(selectAnnotationClasses, annotationClass)) {

I don't have to go an check what contains() method does. It's obvious. 
If there was only the following:

         if (!contains(selectAnnotationClasses, annotationClass)) {

I would immediately jump up with questions: What about when 
selectAnnotationClasses is null? Will there be NPE, false or true 
answer? Let's check what contains() does...

The usage might seem simplified, but it would be too simplified in my 
opinion. Now if there were 2 such usages or more, it would be enough to 
grant creating such method and naming it 'isNullOrContains()' or 
similar, but for a single usage I think this strikes the right balance 
between explicit and concise coding.

>   - parseAnnotation2 seems a bad name; it seems just overloading
> parseAnnotation is good.

Even here I followed the prior art. Since public parseAnnotations() (and 
now also package-private parseSelectAnnotations()) delegates to private 
parseAnnotations2() to do most of the work, I inherited the style and 
made package-private parseAnnotation() delegate to private 
parseAnnotation2() to do the parsing. While I agree using '2' suffix (or 
'0' as in j.l.Class and friends or '_' prefix somewhere else) could be 
replaced with overloading the same name, I also think that using 
distinct names makes code easier to follow mentally. But I don't like 
'2' or '0' or '_' either. So what do you suggest? Maybe 
privateParseAnnotation(s), or parseAnnotation(s)Impl? It makes sense to 
overload names in public API but for internal private usage I think it's 
best to use distinct names (if not for other things, because it's easier 
to refer to a specific method in texts like messages on mailing list, 
bug reports, stack-traces, etc...).

So should we change '2' to some prefix/suffix word then?

Regards, Peter

>
> -Aleksey.
>