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)
- Layout.isEmpty() {
return layout.isEmpty();
}
-
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.
- Code.java : Arrays.copyOf and/or copyOfRange could be used rather than
separate allocation and System.arrayCopy.
- Package.java.stripAttributeKind() : could be a switch.
- Package.java.File : possibly static class and final (which would eliminate
the complaint about File.equals()) ?
- PropMap._listeners could be final (and probably a List rather than explicitly
an ArrayList).
- PropMap.java : convert newly added final to ARM?
- FixedList.java : Anything saved by extending AbstractList rather than
implementing List?
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
>>>
>
