Hi Remi, Thank you for the feedback 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); > } > That is fine, I can make the change. > In SerialDataLink, do you really need readObject/writeObject given > that you call the default implementations. I thought about that but had decided to add them for consistency > > 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. OK > 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); Yeah, long story, but I forgot to reset to diamond syntax (will tell you over a beer sometime :-) ) > in setWarning(), you can use the diamond syntax as the original source does. > and in readObject, you can use the diamond syntax too. OK > 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). I thought about that, but figured since it was already through that check when the object was created, I did not think it required repeating in the readObject method > Also, you should add a comment that because you call getClass() on obj, > there is an implicit null check. Would it be better to put an null check in explicitly? > > 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. I will do this in a separate bug, trying to keep things clear on the bug > > In SerialRef.equals, again, if(...) return should be transformed into return > ... OK Thank you again, will send an update webrev over the weekend Best Lance > >> >> 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
