RFR 8231314: java.time serialization warning cleanup
Roger Riggs
Roger.Riggs at oracle.com
Mon Sep 23 14:56:14 UTC 2019
Hi,
Updated to wrap long lines and remove unneeded @SuppressWarnings.
http://cr.openjdk.java.net/~rriggs/webrev-warn-serializable-8231314-2/
On 9/23/19 4:44 AM, Peter Levart wrote:
> Once more, for the list (sorry)...
>
> Hi,
>
> On 9/21/19 12:31 PM, Chris Hegarty wrote:
>> Roger,
>>
>>> On 20 Sep 2019, at 19:51, Roger Riggs <Roger.Riggs at oracle.com> wrote:
>>>
>>> Please review code cleanup that will remove the need to suppress
>>> soon to be introduced
>>> warnings [1] about static typing of serialization related fields.
>>> A few of the implementation Ser classes that serialize java.time
>>> types are affected.
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-warn-serializable-8231314/
>> I think the change is fine, just a few comments..
>>
>> - AbstractChronology is not necessarily is not always Serializable,
>> but writeReplace expects that it is.
>> Since this is a package-private method, then I assume that it will
>> only ever be called by Serializable subtypes or the serialization
>> framework itself.
Correct, chrono/Ser is only used for built-in subtypes of
AbstractChronology.
I think there is a separate design choice that should be questioned.
In retrsospect I think it should required subclasses of
AbstractChronology be Serializable.
A number of java.time. serializable types can refer to a Chronology and
if Chronology is not
serializable, then they won't be either. If AbstractChronology extended
Serializable,
other implementations would be aware of the expectation and have to make
a deliberate choice.
But this is a separate issue.
>>
>> - time/chrono/Ser.java : it’s kinda awkward to have to cast the
>> return of readExternal, especially since the set of types is locked
>> down, but without these readExternal methods returning xxxImpl types
>> there is little choice.
True
>
> readExternal package-private methods could return Serializable, but
> then those methods would have to cast. Perhaps this would be better as
> it is "closer" to the real impl.
That would be better, but except for lambdas, I don't know of a way to
cast to multiple interfaces.
>
> Another option (as I mentioned in previous message) would be to simply
> mark the Ser field as transient. The fact that currently it always
> holds an instance that *is* Serializable is just a coincidence,
> because it acts as a serialization proxy for Serializable
> implementations. Nothing in the logic of Ser class "requires" the
> instance assigned to the field to be in fact Serializable.
That makes sense for the general non-serializable case, but in the case
of chrono/Ser, all of the implementations
are known to be Serializable.
Thanks, Roger
>
> Regards, Peter
>
>
More information about the core-libs-dev
mailing list