RFR 7118066: Warnings in java.util.concurrent package

[David Holmes]

Chris,

On 7/12/2011 10:09 PM, Chris Hegarty wrote:
> 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 also think the SynchronousQueue has no business being used in this 
test. Though I confess I'm unclear what this test is for - is it for 
emptyIterator() or for "iterators of empty collections" ?

David
-----

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