JDK 13 RFR of JDK-8146726 : Improve AbstractProcessor to issue warnings for repeated information

[Stuart Marks]

Hi Joe,

Some nitpicky comments for you in AbstractProcessor.java. :-)

I'm finding the boolean flags to be fairly confusing. Consider the 'initialized' 
field. This has a synchronized getter; getSupportedAnnotationTypes() calls the 
getter and stores the result in a local for use within this method. However, 
arrayToSet() accesses the 'initialized' field directly, and it's called 
(unsynchronized) from getSupportedOptions() as well as 
getSupportedAnnotationTypes().

First, there's a potential memory visibility problem here, if you're concerned 
about multithreaded access. (At least some of the code in this class seems to 
try to handle thread-safety.) Second, the 'initialized' state is accessed from 
two different levels in the method call hierarchy, e.g. 
getSupportedAnnotationTypes() and arrayToSet(). This isn't necessarily a 
problem, but it does make me pause to try to think about what it means.

Another flag is 'stripModulePrefixes' (parameter to arrayToSet()) and its 
dependent local variable 'stripped' which is set if anything was actually 
stripped from this element. These make arrayToSet() a bit of a puzzle for me. I 
think the following are true:

1) if we aren't stripping module prefixes, we'll issue a warning on any duplicates.

2) if we are stripping module prefixes, we won't issue a warning if a prefixes 
was stripped from this element.

My first inclination is to think that there are really two different 
arrayToSet() methods wanted here. The first doesn't strip and may issue 
warnings, and the second does strip and doesn't issue warnings. It seems to me 
that two separate methods might be simpler than one bigger method with a bunch 
of flags.

But that's not what the current implementation does. However, the current 
implementation doesn't quite make sense to me either. For example, the comment 
says that if module names are stripped, warnings aren't issued because they'd be 
spurious if the array is ["foo/a.B", "bar/a.B"]. However, I'm not sure this 
logic makes sense. It seems to me that if the array is

     ["a.B", "foo/a.B"]

then no warning is issued, but if the array is

     ["foo/a.B", "a.B"]

the a warning is issued. Am I reading this correctly, and is this the right 
behavior?

s'marks


On 4/25/19 8:56 AM, Joe Darcy wrote:
> PS Amended webrev at
> 
>      http://cr.openjdk.java.net/~darcy/8146726.5/
> 
> I added a bit of logic to avoid issuing a duplicate warning when the module name 
> is stripped off to avoid spurious warnings in a case like
> 
>      "foo/a.B"    "bar/a.B"
> 
> The intention of the revision of the semantics of this method in JDK 9 was to 
> allow convenient and warning-free usage with and without modules.
> 
> Thanks,
> 
> -Joe
> 
> On 4/24/2019 7:08 PM, Joe Darcy wrote:
>> Hello,
>>
>> Returning to JDK-8146726, based on previous review feedback I've changed the 
>> title to "Improve AbstractProcessor to issue warnings for repeated 
>> information" and reworked the fix:
>>
>>     http://cr.openjdk.java.net/~darcy/8146726.4/
>>
>> Thanks,
>>
>> -Joe
>>
>>