Code Review Request for Bug #4802647

[Mike Duigou]

This looks complete to me.

Mike

On Dec 19 2011, at 10:53 , Brandon Passanisi wrote:

> Hello core-libs.  I was wondering if somebody could please review the following updated webrev for this bug.  There was an e-mail thread regarding the first webrev [1] that prompted the changes in this webrev.
> 
>   Webrev:  http://cr.openjdk.java.net/~mduigou/4802647/1/webrev/
>   Bug URL: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4802647
> 
> Here is a summary of the changes:
> ...
> 4. The added classes NewAbstractColection and NewAbstractSet were intentionally written to be very basic and were inspired from the source in the bug report.  Is there a better way to write these classes or are they ok as-is?

Seems good to me. 


> 5. The changes to MOAT.java in this updated webrev result in more classes that exhibit the same bug behavior described in this bug report for AbstractCollection (no NPE thrown).  These classes are: java.util.ArrayList and java.util.concurrent.CopyOnWriteArrayList.  Should a new bug be filed for these cases?

If these other classes are just inheriting the AbstractCollection behaviour then the bug can be expanded to include them. If they have their own implementations which also exhibit the problem then they should probably addressed separately. (CopyOnWriteArrayList in particular).


> Thanks.
> 
> 
> [1]: http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-November/008283.html
> 
> 
> On 12/2/2011 2:34 AM, Alan Bateman wrote:
>> On 01/12/2011 22:42, Brandon Passanisi wrote:
>>> Hi Jason.  Thanks for your response.  I was thinking about how I can improve the test using your suggestion. I could possibly do the following:
>>> 
>>>   1. Find all of the subclasses of AbstractCollection which override
>>>   removeAll(Collection<?>) and which also contain the spec language
>>>   which specifies that NullPointerException is thrown if the specified
>>>   collection is null.
>>> 
>>>   2. Find all of the subclasses of AbstractCollection which override
>>>   retainAll(Collection<?>) and which also contain the spec language
>>>   which specifies that NullPointerException is thrown if the specified
>>>   collection is null.
>>> 
>>>   3. Add the classes found in #1 and #2 to the test.
>>> 
>>>   4. If any of the new classes added because of  #3 result in a test
>>>   failure, it might be a good idea to file a new bug as Bug #4802647
>>>   specifically mentions subclasses of AbstractCollection which do not
>>>   override remainAll, retainAll.
>>> 
>>>   5. The public subclasses of AbstractCollection which do not override
>>>   removeAll, retainAll (probably) shouldn't be included in the test as
>>>   the currently existing NewAbstractCollection represents this scenario.
>>> 
>>> What do you think?
>>> 
>> Brandon - it's probably worth getting familiar with the existing tests, in in particular Martin's "Mother Of All Test" (Collection/MOAT.java).
>> 
>> -Alan.
> 
> -- 
> Oracle <http://www.oracle.com>
> Brandon Passanisi | Principle Member of Technical Staff
> 
> Green Oracle <http://www.oracle.com/commitment> Oracle is committed to developing practices and products that help protect the environment