RFR: 8299444 java.util.Set.copyOf allocates needlessly for empty input collections [v2]

[Stuart Marks]

On Sun, 22 Jan 2023 15:20:18 GMT, Attila Szegedi <attila at openjdk.org> wrote:

>> Viktor Klang has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - 8299444: java.util.Set.copyOf allocates needlessly for empty input collections
>>    
>>        Modifies ImmutableCollections.listCopy:
>>        Introduces a check for isEmpty to avoid allocation in the case of an empty input collection.
>>  - 8299444: java.util.Set.copyOf allocates needlessly for empty input collections
>>    
>>    Modifies Map.copyOf:
>>    Introduces a check for isEmpty to avoid allocation in the case of an empty input Map.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 174:
> 
>> 172:             return List.of();
>> 173:         } else {
>> 174:             return (List<E>)List.of(coll.toArray()); // implicit nullcheck of coll
> 
> The comment is no longer relevant here, as it now happens on line 171.

Thanks @szegedi for catching this and @viktorklang-ora for fixing it. I like having comments like this in cases where we need to throw NPE for null and for which there's no explicit `Objects.requireNonNull`. We've had cases in the past where an apparently innocuous refactoring postponed an implicit nullcheck, which opened the possibility of a side effect occuring before NPE was thrown (violates failure idempotency). So I think maintaining such comments is important.

With that in mind, for the Set and Map cases, could you (Viktor) add similar comments there? Arguably they should have been there already, but, oh well, they weren't. Thanks.

-------------

PR: https://git.openjdk.org/jdk/pull/11847