RFR 7118066: Warnings in java.util.concurrent package

[Chris Hegarty]

Thanks David and Doug,

So we should be ready to integrate this change, right? NO, not yet!

There is a failing regression test, java.util.Collections.EmptyIterator. 
This test does a reference comparison ( '==' ) on the iterator returned 
from an empty SynchronousQueue and the iterator returned by 
Collections.emptyIterator(). These are no longer equal since 
SynchronousQueue implements its own EmptyIterator (requires to be 
buildable with 1.6). In fact, there are reference comparisons for 
emptyEnumeration and emptyMap too.

I think this is too restrictive and I don't believe any of the 
collection classes are specified in such a manner. This check should 
simply be removed, unless it was though to be some kind of 'best 
implementation practice', in which case we could just use a different 
collection type in the test.

I you agree I'll file a new CR on this ( I think it may deserve its own 
CR, seems out of scope of 7118066 ). Here are my initial proposed diffs:

diff -r 97e37d3a2ae3 test/java/util/Collections/EmptyIterator.java
--- a/test/java/util/Collections/EmptyIterator.java     Wed Dec 07 
10:17:34 2011 +0000
+++ b/test/java/util/Collections/EmptyIterator.java     Wed Dec 07 
12:00:43 2011 +0000
@@ -59,14 +59,12 @@ public class EmptyIterator {
      }

      <T> void testEmptyEnumeration(final Enumeration<T> e) {
-        check(e == emptyEnumeration());
          check(! e.hasMoreElements());
          THROWS(NoSuchElementException.class,
                 new F(){void f(){ e.nextElement(); }});
      }

      <T> void testEmptyIterator(final Iterator<T> it) {
-        check(it == emptyIterator());
          check(! it.hasNext());
          THROWS(NoSuchElementException.class,
                 new F(){void f(){ it.next(); }});
@@ -75,7 +73,6 @@ public class EmptyIterator {
      }

      void testEmptyMap(Map<Object, Object> m) {
-        check(m == emptyMap());
          check(m.entrySet().iterator() ==
                Collections.<Map.Entry<Object,Object>>emptyIterator());
          check(m.values().iterator() == emptyIterator());
@@ -110,11 +107,6 @@ public class EmptyIterator {

      <E> void testEmptyCollection(final Collection<E> c) {
          testEmptyIterator(c.iterator());
-
-        check(c.iterator() == emptyIterator());
-        if (c instanceof List)
-            check(((List<?>)c).listIterator() == emptyListIterator());
-
          testToArray(c);
      }


-Chris.


On 07/12/2011 06:04, David Holmes wrote:
> Thanks Doug and Chris.
>
> They were just nits so it is fine to proceed - thanks for changing the
> annotations though.
>
> David
>
> On 7/12/2011 11:33 AM, Doug Lea wrote:
>> Thanks for all the comments. We changed to having the
>> annotations on their own lines (even though it furthers
>> the Java tendency of gratuitously occupying too much vertical
>> space :-). Thanks to Chris for explaining why we didn't
>> incorporate some of the other suggestions.
>>
>> Also ...
>>
>>>> - you added:
>>>> * @param s the stream
>>>> for readObject, but not for writeObject. Seems unnecessary for either.
>>>
>>> Right, this is obviously not public API and does seem unnecessary.
>>> This is just
>>> a minor style/comment nit to be consistent with other j.u.c. classes.
>>> But now I
>>> see there are a few other readObject methods that are not consistent
>>> too ( as
>>> well as some writeObjects ). If it's ok we can catch these at another
>>> time?
>>
>> Yes, one of these days we should uniformly just remove them all.
>>
>> -Doug