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
