Re: Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
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
Re: Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
On 11/03/2012 03:11 PM, Lance Andersen - Oracle wrote:
I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01,
taking into account the vast majority of Remi's suggestions.
in SerialJavaObject, hasStaticFields doesn't work, the original code
doesn't work because
it only check for fields that are declared static but not for fields
that are by example public static.
private static boolean hasStaticFields(Field[] fields) {
for (Field field : fields) {
if ( Modifier.isStatic(field.getModifiers())) {
return true;
}
}
return false;
}
This may cause compatibility issue because despite the specification,
the original code
will let objects that have a static field to be serialized.
Also, in readObject, if obj is null, the code should throw an
IOException because
it's not possible to create a SerialJavaObject with null has parameter
(because obj.getClass()
that implictly checks null in the constructor).
All other classes are Ok for me.
I also added SerialStruct to the webrev.
SerialStruct is Ok for me.
Have a great weekend.
Have a nice weekend too.
Best
Lance
cheers,
Rémi
Re: Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
On Nov 3, 2012, at 11:34 AM, Remi Forax wrote:
On 11/03/2012 03:11 PM, Lance Andersen - Oracle wrote:
I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01,
taking into account the vast majority of Remi's suggestions.
in SerialJavaObject, hasStaticFields doesn't work, the original code doesn't
work because
it only check for fields that are declared static but not for fields that are
by example public static.
private static boolean hasStaticFields(Field[] fields) {
for (Field field : fields) {
if ( Modifier.isStatic(field.getModifiers())) {
return true;
}
}
return false;
}
This may cause compatibility issue because despite the specification, the
original code
will let objects that have a static field to be serialized.
I cannot make the change above as it breaks too many tests and I would prefer
to go with the less is more scenario. As I think I mentioned before, I do not
think the original authors really thought through these classes and thankfully
they are not used much, if at all.
Also, in readObject, if obj is null, the code should throw an IOException
because
it's not possible to create a SerialJavaObject with null has parameter
(because obj.getClass()
that implictly checks null in the constructor).
I made the change to readObject. I did not put an explicit check in the
constructor but will do that under a separate bug
I also added the comment to SerialDataLink and removed the read/writeObject
http://cr.openjdk.java.net/~lancea/8002212/webrev.02
Best
Lance
All other classes are Ok for me.
I also added SerialStruct to the webrev.
SerialStruct is Ok for me.
Have a great weekend.
Have a nice weekend too.
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
Re: Review request 8002212 - adding read/writeObject to additional SerialXXX classes -- Updated
On 11/03/2012 05:23 PM, Lance Andersen - Oracle wrote:
On Nov 3, 2012, at 11:34 AM, Remi Forax wrote:
On 11/03/2012 03:11 PM, Lance Andersen - Oracle wrote:
I revised the webrev, http://cr.openjdk.java.net/~lancea/8002212/webrev.01,
taking into account the vast majority of Remi's suggestions.
in SerialJavaObject, hasStaticFields doesn't work, the original code doesn't
work because
it only check for fields that are declared static but not for fields that are
by example public static.
private static boolean hasStaticFields(Field[] fields) {
for (Field field : fields) {
if ( Modifier.isStatic(field.getModifiers())) {
return true;
}
}
return false;
}
This may cause compatibility issue because despite the specification, the
original code
will let objects that have a static field to be serialized.
I cannot make the change above as it breaks too many tests and I would prefer
to go with the less is more scenario. As I think I mentioned before, I do not
think the original authors really thought through these classes and thankfully
they are not used much, if at all.
Also, in readObject, if obj is null, the code should throw an IOException
because
it's not possible to create a SerialJavaObject with null has parameter (because
obj.getClass()
that implictly checks null in the constructor).
I made the change to readObject. I did not put an explicit check in the
constructor but will do that under a separate bug
I also added the comment to SerialDataLink and removed the read/writeObject
http://cr.openjdk.java.net/~lancea/8002212/webrev.02
Ok, thumb up.
Best
Lance
cheers,
Rémi
All other classes are Ok for me.
I also added SerialStruct to the webrev.
SerialStruct is Ok for me.
Have a great weekend.
Have a nice weekend too.
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
