RFR: 8021591 : (s) Additional explicit null checks

[David Holmes]

On 3/08/2013 3:20 AM, Mike Duigou wrote:
>
> On Aug 1 2013, at 16:05 , David Holmes wrote:
>
>> On 2/08/2013 1:57 AM, Alan Bateman wrote:
>>> On 26/07/2013 16:31, Mike Duigou wrote:
>>>> Hello all;
>>>>
>>>> This patch adds some missing checks for null that, according to
>>>> interface contract, should be throwing NPE. It also improves the
>>>> existing tests to check for these cases.
>>>>
>>>> http://cr.openjdk.java.net/~mduigou/JDK-8021591/0/webrev/
>>>>
>>>> The changes to
>>>> src/share/classes/java/util/concurrent/ConcurrentHashMap.java will be
>>>> synchronized separately with the jsr166 workspace. They are part of
>>>> this review to avoid test failures.
>>>>
>>>> Mike
>>> As retainAll and removeAll are long standing methods, are there are
>>> cases where we might now throw NPE when we didn't previously? I'm just
>>> wondering if any of these need to be looked at more closely, minimally
>>> to get into release/compatibility notes.
>
> Yes, this would definitely be something we want to mention for the release notes.
>
>> I get a sense of deja-vu here. For retainAll/removeAll this fixes the case where you would not get NPE if the target collection is empty. We already dealt with this for some j.u.c collections - see 7123424 and then 8001575.
>
> I am not sure if this is a vote for or against continuing to add the missing NPE checks. If we decide against adding the checks I would actually push for revising the spec (make the NPE optional) to reflect the relatively common practice and possibly even remove some of the recently added NPE.

It was merely an observation that we have already trodden this path. But 
if it were a vote I would vote to throw and add a note to the 
compatibility docs.

A spec that is loosened to say may throw NPE is useless because anyone 
who relies on an implementation not throwing will be bitten by an 
implementation that does.

Cheers,
David
-----

> Mike
>