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

[Kumar Srinivasan]

Hi Mike,

Thanks for the review, here is the new version:
http://cr.openjdk.java.net/~ksrini/6990106/webrev.02/

> Attribute.java/Instruction.java/Package.java.File :
>
> - Layout.equals(Object x) {
>    return x instanceof Layout&&  equals((Layout)x);
> }
>
> should be :
>
> Layout.equals(Object x) {
>    return (null != x)&&  (x.getClass() == Layout.class)&&  equals((Layout)x);
> }
>
> as sub-classes also using instanceof would have a non-reflexive equals(). ie. layout.equals(someSubClassOfLayout) != someSubclassOfLayout.equals(layout)
Fixed.

> - Layout.isEmpty() {
>   return layout.isEmpty();
> }

Fixed

> - BandStructure.java/ClassReader.java/Coding.java/CodingChooser.java/PackageReader.java/PackageWriter.java : is there a reason to use concrete types (HashMap, ArrayList, HashSet) for fields and vars such as basicCodingIndexes, allKQBands? It seems not.

Fixed most of these, some of them need to be concrete.

> - Code.java : Arrays.copyOf and/or copyOfRange could be used rather than separate allocation and System.arrayCopy.
Fixed
> - Package.java.stripAttributeKind() : could be a switch.
Fixed
> - Package.java.File : possibly static class and final (which would eliminate the complaint about File.equals()) ?

This can be made final but not static, due to references.

> - PropMap._listeners could be final (and probably a List rather than explicitly an ArrayList).

Fixed.
> - PropMap.java : convert newly added final to ARM?

I will pass on it right now, there are other places too, which needs 
ARM'ing, but
  talking to Stuart Marks he seems to have the ARM'izing effort on his 
agenda.

> - FixedList.java : Anything saved by extending AbstractList rather than implementing List?

Done

Thanks
Kumar

> On Nov 15 2010, at 17:24 , Kumar Srinivasan wrote:
>
>> 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
>>>>