Code Review Request for Bug #4802647

[David Holmes]

On 22/11/2011 4:29 AM, Brandon Passanisi wrote:
> 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?

Your fix is correct, but I'm not sure that fixing this bug is the best 
thing to do here. You could easily argue to amend the spec instead. That 
said, Josh Bloch already indicated (back in 2003) in the CR that this 
should be fixed.

I'd like to hear the opinions of other Collections experts.

David


> 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.
>