Hi Stuart,

I believe the code in its current state out in review is a good trade-off of effort needed to address the warnings vs improvement in the quality of the code.

Certainly the code would benefit from a larger cleanup, but I agree that is not warranted at this point. Longer term, say for JDK 9, we should probably look to providing this functionality in some other way.

Approved to go back.

Thanks,

-Joe

On 08/07/2013 10:01 AM, Lance Andersen - Oracle wrote:
Hi Stuart,

On the surface the changes look fine, including what you did for SwitchData.

Certainly another pair of eyes would be good also on this.

Best
Lance
On Aug 7, 2013, at 2:28 AM, Stuart Marks wrote:

Hi,

Please review the fix for this warnings cleanup bug.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022479
    (not yet available publicly, but should be shortly)

Webrev: http://cr.openjdk.java.net/~smarks/reviews/8022479/webrev.0/

There are a few items of note.

This is *very* old code. You can see some of the authors' names in the code. 
It's replete with uses of Vector, Hashtable, and Enumeration. It also has a 
kind of musty style, which might have been reasonable from the standpoint of C 
programmers (which we all were at the time!). For example, there are a bunch of 
cases of direct field access to another class. There are also cases where a 
class contains a Vector that's returned by a getter method. I guess we just 
have to trust the caller not to modify it at the wrong time.

I've confined most of my cleanups to the addition of generics (which gets rid of the 
rawtypes and unchecked warnings). There were a smattering of other warnings I've cleaned 
up as well. I've also replaced few cases of calls to "new" on boxed types like 
Integer and Long with calls to the respective valueOf() methods. I didn't check all the 
code for this, though, just instances I happened to run across.

There is much more cleanup that could be done that I've elected not to do, such 
as replacing Vector, Hashtable, and Enumeration with ArrayList, HashMap, and 
Iterator, and using the enhanced-for loop. These changes would *probably* be 
safe, but they would require more analysis and testing than I'm willing to do 
at the moment.

Two locations deserve a bit of scrutiny.

1) There are four occurrences in Assembler.java of calls to 
MemberDefinition.getArguments(). Unfortunately MemberDefinition resides in the 
sun.tools.java package and getArguments() returns a raw Vector. This is also 
overridden in a couple places spread across a couple sun.tools packages, and 
there are hints that it being a LocalMember or MemberDefinition. It seems out 
of scope to try to fix up the return value of getArguments() and its various 
overrides.

Locally in the Assembler.java file, though, the contents of the returned vector are 
always cast to MemberDefinition, so it seems sensible to make an unchecked cast of 
the getArguments() return value to Vector<MemberDefinition> and to suppress 
warnings at that point. Usage beyond those points within Assembler.java is filled 
with sweet and juicy generic goodness.

2) SwitchData.java contains a field whereCaseTab which is a Hashtable<Integer,Long> ... 
most of the time. In the addTableDefault() method, a value is put into the Hashtable under the 
key "default" -- a String. Ick.

To deal with this I declared it as Hashtable<Integer,Long> since that's the typical case, 
and for the put(String, Long) case I cast through raw and suppressed the warning. The get() 
method already takes an Object so we're OK there. Note that "default" as a key for 
this Hashtable is used in a couple other places in Assembler.java. It's not intractable to 
change these to some other sentinel value, but, you know, where does one stop?

(I said "Ick" when encountering a mix-type collection, and in a generified 
world we very rarely see this style anymore. Upon further reflection, though, from the 
standpoint of a pre-generics world, this is pretty sensible. Using a string as a sentinel 
key is guaranteed not to collide with any Integer keys. Sometimes it's possible to use -1 
or Integer.MAX_VALUE as a sentinel, but if any integer value is permitted as a key, what 
does one do then?)

With this changeset, sun.tools.asm is reduced from about 76 to zero warnings.

Thanks,

s'marks


Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com


Reply via email to