RFR: 8021591 : (s) Additional explicit null checks

[Martin Buchholz]

My feeling is that the JDK specs have been creeping in the direction of
excessive pedantry and doc lawyerism.  I think it's overall a benefit of
Java that its documentation is more readable (at the cost of being a little
less precise) than the typical ISO spec.

So we usually say "returns true if the foo got frozzled" instead of the
more precise "returns true if and only if the foo got frozzled".
It's "obvious" that an NPE is thrown on an attempt to derefence a null arg,
which might not happen in some corner case.  I think it's overall a
detriment if either extra null checks are inserted or the spec is made more
precise but less readable.

Consider the Throws spec for
http://download.java.net/jdk8/docs/api/java/util/TreeMap.html#containsKey(java.lang.Object)

ClassCastException - if the specified key cannot be compared with the keys
currently in the map

That's incomplete and imprecise, but don't pessimize by adding code to
compare the arg with every key in the map!  (which is the equivalent of
what is happening here)

A vote from me for maintaining the already high level of pedantry we have,
but not raising it any further.



On Mon, Aug 19, 2013 at 3:15 PM, Mike Duigou <mike.duigou at oracle.com> wrote:

>
> On Aug 1 2013, at 08:57 , 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?
>
> Yes.
>
> retainAll(null) and removeAll(null) will more consistently throw NPE.
> Previously the NPE was not thrown by some collections when the collection
> they were empty.
>
> > I'm just wondering if any of these need to be looked at more closely,
> minimally to get into release/compatibility notes.
>
> I will definitely tag this issue for release notes mention.
>
> > -Alan
>
>