Review request 8002212 - adding read/writeObject to additional SerialXXX classes
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 Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com
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@oracle.com
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@oracle.com
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com
On 11/03/2012 01:46 AM, Lance Andersen - Oracle wrote:
Hi Remi,
[...]
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
The serialization implementation needs to create metadata associated with readObject/writeObject, so if we can avoid to implement them, we reduce the runtime memory footprint a little. I would prefer to have a comment saying that default serialization is Ok here.
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 :-) )
sure, now or you have to visit Paris or I have to go to NY :)
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
A serialization stream can be forged to encode a SerialJavaObject that references an object that have a static field without creating a SerialJavaObject in memory.
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?
As you prefer :) [...]
Thank you again, will send an update webrev over the weekend
Best Lance
cheers, Rémi
On Nov 3, 2012, at 11:14 AM, Remi Forax wrote:
On 11/03/2012 01:46 AM, Lance Andersen - Oracle wrote:
Hi Remi,
[...]
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
The serialization implementation needs to create metadata associated with readObject/writeObject, so if we can avoid to implement them, we reduce the runtime memory footprint a little. I would prefer to have a comment saying that default serialization is Ok here.
OK, I will just comment the methods out and add a comment as you suggest
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 :-) )
sure, now or you have to visit Paris or I have to go to NY :)
Or Boston, where I am based or perhaps a JavaOne ;-)
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
A serialization stream can be forged to encode a SerialJavaObject that references an object that have a static field without creating a SerialJavaObject in memory.
True and there are tests in the test suite that basically do that. Anyways, I added that check in the revised webrev that I pushed earlier.
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?
As you prefer :)
[...]
Thank you again, will send an update webrev over the weekend
Best Lance
cheers, Rémi
Best Lance Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com
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@oracle.com
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 Lance.Andersen@oracle.com
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
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.Andersen@oracle.com
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
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.Andersen@oracle.com
participants (2)
-
Lance Andersen - Oracle
-
Remi Forax