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