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.ri...@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
