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

Reply via email to