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

[Lance Andersen - Oracle]

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.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