Code Review Request for Bug #4802647

[David Holmes]

Hi Brandon,

On 19/11/2011 11:21 AM, Brandon Passanisi wrote:
> Hello core-libs-dev at openjdk.java.net. Please review the following patch
> to fix Bug 4802647 (coll) NullPointerException not thrown by
> AbstractCollection.retainAll/removeAll: The fix is quite small and I
> have included a test program, all of which can be found within the
> following webrev:
>
> http://cr.openjdk.java.net/~mduigou/4802647/0/webrev/

The bug report is a little misleading. The bug only exists when the 
current collection is empty - in which case we skip most of the method 
body. Otherwise we will generate the NPE when we call 
c.contains(iter.next()). The CR should be updated to note this.

So while your fix is correct, I hate to see good code penalized 
(explicit null check) to allow bad code to throw exceptions as required. 
But I don't see a way around it.

With regard to the test program - it can be simplified somewhat as 
unexpected exceptions can just be allowed to propagate and failures to 
throw can be detected inline:

      public static void main(String[] args) {

          // Ensure removeAll(null) throws NullPointerException
          try {
              (new NewAbstractCollection<>()).removeAll(null);
              fail("NullPointerException not thrown for removeAll(null).");
          } catch (NullPointerException expected) {
          }

          // Ensure retainAll(null) throws NullPointerException
          try {
              (new NewAbstractCollection<>()).retainAll(null);
              fail("NullPointerException not thrown for retainAll(null).");
          } catch (NullPointerException expected) {
          }
      }

Cheers,
David

> Thanks.