I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01, taking into account the vast majority of Remi's suggestions.
I also added SerialStruct to the webrev. Have a great weekend. Best Lance On Nov 2, 2012, at 7:42 PM, Remi Forax wrote: > On 11/02/2012 11:57 PM, Lance Andersen - Oracle wrote: >> This is similar to 8001536, just additional classes. >> >> This adds read/writeObject, equals, clone methods to additional SerialXXX >> classes >> >> SQE, JCK and JDBC Unit tests all pass. >> >> The webrev can be viewed at >> http://cr.openjdk.java.net/~lancea/8002212/webrev.00 > > Hi Lance, > in SerialArray.equals(), I prefer > > return baseType == sa.baseType && > baseTypeName.equals(sa.baseTypeName)) && > Arrays.equals(elements, sa.elements); > > instead of > > if(baseType == sa.baseType && baseTypeName.equals(sa.baseTypeName)) { > return Arrays.equals(elements, sa.elements); > } > > In SerialDataLink, do you really need readObject/writeObject given > that you call the default implementations. > > in SerialJavaObject, in equals, you should declare a local variable like in > SerialDataLink.equals, > even if the local varialble is used once, it's more readable. > Also like in SerialArray.equals, you can do a return directly instead of > if(...) return true. > in clone(), you can use the diamond syntax, > sjo.chain = new Vector<>(chain); > in setWarning(), you can use the diamond syntax as the original source does. > and in readObject, you can use the diamond syntax too. > In readObject, you forget to throw an exception if there are some static > fields > in getClass().getFields() as the constructor does > (this code can be moved in a private static method). > Also, you should add a comment that because you call getClass() on obj, > there is an implicit null check. > > This can be fixed as a separated bug or not but the code of > method SerialJavaObject.getField() is weird, the code checks if fields can be > null, > but fields is never null. Also, cloning the field array is perhaps a better > idea > if the reflection implementation doesn't cache the array of fields. > > In SerialRef.equals, again, if(...) return should be transformed into return > ... > >> >> Best >> Lance > > cheers, > Rémi > >> >> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 >> Oracle Java Engineering >> 1 Network Drive >> Burlington, MA 01803 >> lance.ander...@oracle.com >> >
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
