JDK 12 RFR of JDK-8146726 : Refactor AbstractProcessor to use Set.of and related methods

[Stuart Marks]

On 11/28/18 6:09 PM, joe darcy wrote:
> Adding Stuart for a collections consult; Stuart, what is the currently 
> recommended idiom to construct a unmodifiable collection with the same elements 
> as a existing collection? In particular, for a set of Strings is something like
> 
>      Set.of(stringSet(new String[0]))
> 
> preferred over
> 
>      Collections.unmodifiableSet(stringSet);
> 
> Assume serialization is not a concern.

If the resulting set is intended to be long-lived, then Set.of(...) or 
Set.copyOf(coll) will make an unmodifiable copy of the source elements.

Note that Set.copyOf() [new in JDK 10] will make a set by copying the elements 
from the source collection, silently compressing out duplicates. This differs 
from Set.of(...) which as Ron noted will throw if it encounters duplicates.

So, a simple way to do what you want is to get the value as a String[], wrap it 
in a List, and then call Set.copyOf():

     return Set.copyOf(Arrays.asList(so.value()));

But, see below.

> On 11/27/2018 1:23 PM, Ron Shapiro wrote:
>> Set.of() throws if there are duplicate elements - but arrayToSet() didn't do 
>> so. Is it intended to throw if either of these take duplicates?
>>
> 
> Good catch.
> 
> After some pondering, I think it is preferable for AbstractProcessor to not 
> treat duplicates as an error, but to issue a warning in if duplicates are found. 
> Arguably, it would have been reasonable to treat such condition as an error 
> initially when the API was introduced, but adding such a check now could have 
> some unnecessary behavioral compatibility impact. The case to add an erroneous 
> check for options is stronger than for supported annotations because supported 
> annotations have some additional processing if modules are not present. In more 
> detail, with modules to fully specify an annotation type, the module name needs 
> to be used: "mod1/foo.bar" vs "mod2/foo.bar". If such a processor is run when 
> modules are not used, then the names are stripped to "foo.bar" in both cases and 
> that should not be treated as an error.
> 
> Revised webrev:
> 
>      http://cr.openjdk.java.net/~darcy/8146726.3/
> 
> Will possibly update based on guidance from Stuart on the collections usage 
> question.

One question is how explicit you want the error message to be.

If you're willing to live with a nonspecific "warning: there are duplicates" you 
can do Set.copyOf() as I suggested above and then compare the set's size() to 
the length of the array. If they differ, you know there were duplicates. But you 
don't know which elements were duplicated.

If you want to print the specific options that were duplicated, building up a 
HashSet and printing a warning whenever add() returns false is a reasonable 
approach. (There ways to do this using streams but they aren't significantly 
better for a problem this simple.)

Note IvanG's comment about proper sizing of the HashSet.

Now, if you're going to be creating a HashSet anyway, as part of deduplicating 
the options and issuing warnings, and if the caller is going to do something 
simple (like iterating over it, or just doing a contains check) and throwing it 
away, then it might not be worth copying the HashSet into a separate 
unmodifiable set. So maybe just wrapping it in Collections.unmodifiableSet() 
would be reasonable.

Sorry not be conclusive. :-)

s'marks