On Sun, 22 Jan 2023 15:20:18 GMT, Attila Szegedi <att...@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
