On 2/18/2014 10:05 AM, Daniel Fuchs wrote:
> Thanks Joe,
>
> +1
>
> There's a small alignment issue in SerializationTest.java line 152.
That's a debug I added a moment ago, removed.
>
> Also the
>
> if (condition) {
> }
> else {
> }
>
> formatting in the source files is bizarre, but if it can help with
> future merges I have no issues.
That's from Xerces (they use Eclipse). I've seen them do this in pure
format changesets, that is, they'd change "} else {" to the above.
NetBeans format would have been:
if (condition) {
} else {
}
I personally like The NetBeans format. But it will take a while before
we could get jaxp code formatted properly since in many cases the
relevant files are very big (over 3000 lines in this case). Hitting
NetBeans format can generate a lot of noises to the webrev.
>
> No need to regenerate a webrev.
Thanks,
Joe
>
> -- daniel
>
> On 2/18/14 6:42 PM, huizhe wang wrote:
>>
>> On 2/18/2014 3:39 AM, Daniel Fuchs wrote:
>>> On 2/17/14 11:17 PM, huizhe wang wrote:
>>>> Thanks Daniel.
>>>>
>>>> I updated the test to call class.getResourceAsStream (see fromFile
>>>> method) instead at the runtime. The filePath now is only used for the
>>>> helper method to generate test file. Also added readObject() to
>>>> XMLGregorianCalendarImpl (at the bottom). The method calls the save()
>>>> method to initialize the orig_* fields as done in the constructors.
>>>>
>>>> http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
>>>
>>> Hi Joe,
>>>
>>> The changes look good.
>>>
>>> In the serialization test I wonder if it would be better
>>> to simply replace:
>>>
>>> 53 {
>>> 54 filePath =
>>> SerializationTest.class.getResource("SerializationTest.java").getPath();
>>>
>>> 55 int pos = filePath.lastIndexOf('/');
>>> 56 filePath = filePath.substring(0, pos + 1);
>>> 57 }
>>>
>>> with:
>>>
>>> {
>>> filePath = System.getProperty("test.src", ".");
>>> }
>>
>> Ok, I changed that to:
>>
>> 54 filePath = System.getProperty("test.src");
>> 55 if (filePath == null) {
>> 56 //current directory
>> 57 filePath = System.getProperty("user.dir");
>> 58 }
>> 59 filePath += File.separator;
>>
>>
>> Thanks,
>> Joe
>>
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>>>
>>>> Thanks,
>>>> Joe
>>>>
>>>> On 2/17/2014 8:23 AM, Daniel Fuchs wrote:
>>>>> Hi Joe,
>>>>>
>>>>> Sorry for the late reply...
>>>>>
>>>>> On 2/14/14 6:35 PM, huizhe wang wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> I added a SerializationTest. The test contains a helper that can
>>>>>> generate serialization files for XMLGregorianCalendar and Duration.
>>>>>> I've
>>>>>> created such files for jdk6,7,8 and 9, and manually run the test,
>>>>>> that
>>>>>> is, read them back with JDK6, 7, 8 and 9. The test worked fine.
>>>>>> In the
>>>>>> JDK9(or 10 in the future) repo and for an auto-run, it would use the
>>>>>> current JDK9/10 build and test against JDK6, 7, 8 and 9. Past
>>>>>> JDK10, we
>>>>>> could consider add serialization files for JDK10.
>>>>>>
>>>>>> The new fields did not affect serialization compatibility. The above
>>>>>> tests passed with/without the new fields being transient. But I
>>>>>> added
>>>>>> transient since it's the right thing to do.
>>>>>>
>>>>>> Adding fields is a compatible change in accordance with Java Object
>>>>>> Serialization Spec
>>>>>> <http://docs.oracle.com/javase/7/docs/platform/serialization/spec/version.html#6678>.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Concerning the new test I think it would be much more
>>>>> reliable to open the .ser files as a resource, using
>>>>> something like:
>>>>>
>>>>> InputStream streamIn =
>>>>> SerializationTest.class.getResourceAsStream(
>>>>> javaVersion + FILENAME_CAL);
>>>>> ObjectInputStream objectinputstream = new
>>>>> ObjectInputStream(streamIn);
>>>>>
>>>>> The reason is that URL.getPath() is an URL path - not a file path,
>>>>> so it
>>>>> can contain encoded characters (such as %20) that FileInputStream
>>>>> will
>>>>> not understand. I suspect that if there's a white space or some other
>>>>> special character in the path somewhere your current mechanism
>>>>> will not work.
>>>>>
>>>>> Also it would be good to verify what is the value of the transient
>>>>> fields after deserialization: I see that XMLGregorianCalendarImpl
>>>>> initializes them with non-default values - and you may need to
>>>>> write a readObject() to reinitialize those fields to these non
>>>>> default values: the deserialization framework may simply leave
>>>>> them with the global default for fields of that type (null
>>>>> for objects, 0 for numerical values, false for boolean etc...)
>>>>> [I am not 100% sure but it will not hurt to check]
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Joe
>>>>>>
>>>>>> On 2/13/2014 6:23 AM, Alan Bateman wrote:
>>>>>>> On 13/02/2014 08:18, huizhe wang wrote:
>>>>>>>> Hi Alan, Lance, and Daniel,
>>>>>>>>
>>>>>>>> The Xerces serialization revision meant to create a serialization
>>>>>>>> form that would help maintain future serialization compatibility.
>>>>>>>> But
>>>>>>>> in reality it itself is causing significant incompatibility as
>>>>>>>> Alan
>>>>>>>> pointed out below and we discussed previously. I've removed the
>>>>>>>> revision from the patch as a result.
>>>>>>>>
>>>>>>>> Please see the new webrev here:
>>>>>>>> http://cr.openjdk.java.net/~joehw/jdk9/8033980/webrev/
>>>>>>> Thanks for dropping the serialization change as it was just not
>>>>>>> going
>>>>>>> to work the way you had intended.
>>>>>>>
>>>>>>> I agree with Daniel's comment about all the new fields added to
>>>>>>> XMLGregorianCalendarImpl as it's not clear why they aren't
>>>>>>> transient.
>>>>>>>
>>>>>>> I have not studied the rest of the changes but I think Daniel and
>>>>>>> Lance are.
>>>>>>>
>>>>>>> -Alan
>>>>>>
>>>>>
>>>>
>>>
>>
>