RFR 7118066: Warnings in java.util.concurrent package (original) (raw)
Chris Hegarty chris.hegarty at oracle.com
Wed Dec 7 12:09:39 UTC 2011
- Previous message: RFR 7118066: Warnings in java.util.concurrent package
- Next message: RFR 7118066: Warnings in java.util.concurrent package
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
- Previous message: RFR 7118066: Warnings in java.util.concurrent package
- Next message: RFR 7118066: Warnings in java.util.concurrent package
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]