Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated

[Lance Andersen - Oracle]

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.Andersen at oracle.com
>> 
> 

-------------- next part --------------

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