Please review java.util.jar.pack.* cleanup/refactoring/generificaiton

Kumar Srinivasan kumar.x.srinivasan at oracle.com
Tue Nov 16 01:24:17 UTC 2010


Hi John, et. al.,

This revision contains all fixes noted by Brian below and other
comments from Alan.

http://cr.openjdk.java.net/~ksrini/6990106/webrev.01/

Thanks
Kumar

> Mostly style issues, but at least one likely bug.
>
> In a few places, you've used @SuppressWarnings("unchecked").  While 
> this may well be unavoidable, it is worth factoring this out into the 
> smallest possible chunk.  For BandStructure, you've applied it to the 
> whole class, and there's some big methods that use it too.  You can 
> usually get it down to a small "makeArrayOfGenericFoo" method, since 
> this is the most common source of non-fixable unchecked warnings.
>
> Also using an interface like Constants to import symbols is an 
> antipattern and is better replaced with static imports.
>
> In ClassReader you've replaced use of new Integer() and friends with 
> Integer.valueOf(), but its better style to go all the way and just use 
> autoboxing.
>
> In Instruction the equals() method takes into account bc, w, length, 
> and bytes, but the hashCode() method also takes into account pc.  This 
> may violate the "equal objects must have equal hashcodes" rule.
>
> Throughout you've changed loops from
>   for (Iterator i=...)
> to
>   for (Iterator<T> i=...)
> but didn't go all the way and just use foreach loops.
>
> PropMap should extend TreeMap by composition, not extension.  This 
> implementation is subject to the hazards outlined in the Effective 
> Java item "Prefer composition to extension."  (For example you 
> override put() but not putAll(), but this idiom cannot be made to work 
> without tightly coupling it to the superclass implementation.)
>
> On 11/11/2010 12:29 PM, Kumar Srinivasan wrote:
>> Hi,
>>
>> Appreciate a review of the pack200 cleanup work, in the following areas:
>>
>> 1. General generification of Collections.
>> 2. fixed worthy findbugs items.
>> 3. fixed worthy Netbeans nags.
>> 4. Elimination of array/generics combination.
>>
>> The webrev is here:
>> http://cr.openjdk.java.net/~ksrini/6990106/webrev.00/
>>
>> Thanks
>> Kumar
>>




More information about the core-libs-dev mailing list