RFR: 8021591 : (s) Additional explicit null checks

[Mike Duigou]

On Aug 1 2013, at 09:34 , Martin Buchholz wrote:

> Overall, I hate these kinds of changes to be super-pedantically correct, at the cost of a small performance loss and a small compatibility hit.  I resisted doing them when I was maintaining these classes.  So there's an edge case where an NPE isn't thrown - who cares?

The problem comes up when moving between interface implementations. The problem in removeAll/retainAll is that we have a diversity of behaviours in implementations with some throwing the NPE and some not. The interface has always indicated that the NPE is thrown and while I agree that it's probably not very important if the NPE isn't thrown in singular cases the lack of consistency makes it harder to switch between implementations. This showed up in users switching from ArrayList (which didn't throw the NPE) to CopyOnWriteArrayList (which does since Java 7 throw the NPE). The goal here is to improve conformance to the interface not strictly for pedantry but to foster greater interoperability among implementations.


>  Are there users asking for this?

As mentioned it has most frequently been expressed as a barrier to swapping implementations. There have been a few reports where we were chastised for failing to detect a user's error. "I know I shouldn't have been passing null but I was surprised that the method didn't signal an error."


Mike


> On Fri, Jul 26, 2013 at 4:31 PM, Mike Duigou <mike.duigou at oracle.com> 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
>