RFR 8231314: java.time serialization warning cleanup

[Roger Riggs]

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
>
>