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

[Kumar Srinivasan]

On 11/11/2010 11:06 AM, Brian Goetz wrote:
> 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.

I will look into this, and see if I can break it up.

>
> Also using an interface like Constants to import symbols is an 
> antipattern and is better replaced with static imports.
>
will look into it.

> 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.
will look into it.

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

will look into it.

>
> 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.
ah, there are places where we need "i" those that don't have been 
replaced with
for-each.

>
> 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.)
I will see what I can do here.

Kumar

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