Code Review Request for Bug #4802647

[Brandon Passanisi]

On 11/21/2011 3:58 PM, David Holmes wrote:
> 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

Are there any opinions on this from other Collections experts?

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