On 12/ 6/11 01:12 AM, David Holmes wrote:
Chris, Doug,
A few nits see below.
Cheers,
David
-----
As a matter of style can we ensure annotations are on separate lines. I
find this:
@SuppressWarnings("unchecked") E x = (E) items[takeIndex];
hard to read. (I hate seeing local variable annotations in the first
place - way too much clutter in my opinion.)
local variable annotations/declaration now on new line.
Is the reason for constructs like this:
HashEntry<K,V>[] tab = (HashEntry<K,V>[])new HashEntry<?,?>[cap];
that we can't utilize diamond? Otherwise it would nicely reduce to:
HashEntry<K,V>[] tab = new HashEntry<>[cap];
I see follow up between you, Remi, and Maruizio, on this. Also note that
we need to be able to compile with 1.6, so no diamond anyway. I assume
this code is good as stands?
In ConcurrentSkipListMap:
- doesn't the class-level unchecked annotation make the local-variable
annotation redundant in clone()
Right, but it is not wrong and may be useful if there is ever
refactoring of this code and the class level unchecked annotation is
removed. clone always needs to suppress unchecked. I'm ok either way.
- 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?
- in the KeySet and Values classes why the changes from <E,Object> to
<E,?> ? Seems superfluous. Did it affect any warnings?
Yes, this did effect warnings. The ConcurrentNavigableMap being passed
to the KeySet is of type ConcurrentNavigableMap<K,V> where keySet was
expecting ConcurrentNavigableMap<K,Object>. In this case we could use
<K,V>, but since V is not interesting/used it can simply be anything,
'?'. Ditto for Values, only the value is interesting.
In Exchanger why suppress the serial warning rather than add the
serialVersionId? Elsewhere the id was added. (I prefer to suppress but
am querying consistency here)
Since Node and Slot can never be serialized, but just happen to extend a
Serializable class, it is safe to just suppress the warning here, rather
than giving a bogus value.
In LinkedTransferQueue we changed:
this.<E>cast(item);
to
LinkedTransferQueue.<E>cast(item);
but in ArrayBlockingQueue we simply changed to: (E) item. Were these
cast methods introduced simply to localize the unchecked suppression?
It would appear that the static cast method was added to perform safe
casts without triggering warnings ( similar to java.lang.Class.cast ), a
long time ago. The compiler warnings we were seeing here were simply
that a static method should be qualified by the type name,
LinkedTransferQueue, instead of by an expression. I didn't look into
consistency with other classes.
Also in LinkedTransferQueue we went from:
E e;
while ( (e = poll()) != null) {
to
for (E e; (e = poll()) != null;) {
the side-effect on the loop condition is ugly in a for-loop. Why change
from the while? Why not change to a proper for-loop:
for (E e = poll(); e != null; e = poll()) {
I agree, at least to me this is cleaner. This is just code style cleanup
( or maybe not ;-) ), I'm ok either way, or just dropping it until a
later sync.
In SynchronousQueue:
- the Transferer, TransferStack and TransferQueue classes have been
generified, but not the internal SNode and QNode classes. Generifying
nodes would take a bit of work, but I guess I don't see the point of
generifying the others.
Doug answered this in his earlier mail:
"Yes, possible but it would preclude (or force lying to the type
system about) using sentinels that we often need to use for the sake
of GC/cleanup. So again, by convention, we always use plain Object
in these non-blocking data structures."
- why stop using Collections.emptyIterator()?
Collections.emptyIterator was added in 1.7 ( and also in 6Open? ), so I
assumed Doug wanted to break the reliance on compiling with 1.7 here.
- same comments on while-loop to for-loop conversions as for LTQ
Same answer ;-)
Thanks,
-Chris.
On 6/12/2011 1:36 AM, Chris Hegarty wrote:
Cleanup warnings in the j.u.c. package.
This is a sync up with the warning fixes in Doug's CVS. There are also a
few style cleanups, import fixes, trivial local variable renaming,
typos, etc. But nothing too surprising!
http://cr.openjdk.java.net/~chegar/7118066/webrev.00/webrev/
-Chris.
P.S. I have already reviewed this, and the contribution is of course
from Doug.
