Code Review Request for Bug #4802647

[Brandon Passanisi]

Thank you for the review David.  I'll make the changes to the test 
program as you have suggested and I will also update the bug report with 
the comments you have given.  I'll then send out an updated webrev.  
Just to double-check, when you mention "But I don't see a way around it" 
in regards to the fix to AbstractCollection.java... it sounds like you 
agree to keep the change as-is.  Is this correct?

On 11/20/2011 6:19 PM, David Holmes wrote:
> 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.

-- 
Oracle <http://www.oracle.com>
Brandon Passanisi | Principle Member of Technical Staff

Oracle Java Standards Conformance

Green Oracle <http://www.oracle.com/commitment> Oracle is committed to 
developing practices and products that help protect the environment