RFR 7118066: Warnings in java.util.concurrent package

[Chris Hegarty]

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.