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
