On 17/03/15 11:52, Peter Levart wrote:
On 03/17/2015 10:57 AM, Chris Hegarty wrote:
Peter, Alan,
After further thought I think that requiring all final fields to be set is
reasonable behaviour. Since there is no compile time checking, this is a
reasonable runtime effort to catch what is likely to be developer errors. To
address Alan’s comments, I beefed up the API docs and added examples to typical
usage.
Updated specdiff (and webrev):
http://cr.openjdk.java.net/~chegar/8071472/01/
<http://cr.openjdk.java.net/~chegar/8071472/01/>
This version also includes a number of changes to readObject implementations in
the base module, to replace unsafe usage with this field setter API.
Hi Chris,
It's good that you included changes to some readObject() implementations
in base module as these are the 1st real-world examples of API use. This
way we can get some experience as to whether the API is adequate. This
experience includes subtleties of assignment checking on finals. I think
it would be desirable that if this API is included in JDK9, more
migration from Unsafe to this API is gradually attempted before the API
is frozen for JDK9.
I think we are mainly in agreement with regard to the level of the API.
As with all new API's, the more bake time in the release the better. If
we accept the general approach, then further tweaks can be done before 9
ships.
I plan to file bugs against specific component areas, or by module, to
remove unsafe usage and replace it with this API. There should be enough
time to do this in 9, and the experience from doing so can feedback into
the API, if needed.
As for the changes, I went through them and I think they are OK. Just
one nit: private static FieldSetterContext.finalInstanceFieldCount()
helper method could be moved to inside the FieldSetterContext.FieldsMap
nested class as it is only used from it's constructor. This way you
avoid generating synthetic access methods.
Thanks, done.
Also in ObjectInputStream.fieldSetter() javadoc, in the following statement:
Returns the |FieldSetter| instance associated with current object and
type being deserialized, which gives write access to the *types*
declared instance fields, including final, during the |readObject| callback.
Fixed.
I think it should be changed from: "types" -> "type's" (or class' for
that matter as only classes can have instance fields).
Regards, Peter
-Chris.
