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.
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.
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.
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.
On 13 Mar 2015, at 19:36, Chris Hegarty <chris.hega...@oracle.com> wrote:
On 13 Mar 2015, at 17:58, Peter Levart <peter.lev...@gmail.com> wrote:
On 03/12/2015 11:24 PM, Peter Levart wrote:
So if a readObject calls fields() without calling defaultReadObject() then it
has to set every final field. On one hand that ensures that every final field
is set, on the other hand it is a bit odd because omitting the call to fields()
means they all get their default value.
If fields() is not called, we must be backwards-compatible.
But yes, this constraint is questionable. On one hand it tries to mimic the assignment to
final fields in constructors, but it can't go all the way, as read access to final fields
in readObject() is not limited to just those that have already been assigned (like it is
in constructor with definitive assignment rules). We could add get() methods to
FieldAccess that would constraint the reads of final fields to those that have already
been assigned, but that is just like "advisory locking" - we can only advise
users to use FieldAccess to read fields in readObject() but we can't prevent them from
reading them directly.
So if this doesn't have much sense and brings confusion, it can go away.
...or it can stay in part where we check that a final field is not set more
than once, which can help especially when use of FieldAccess API is combined
with defaultReadObject().
Yes, that makes sense.
The final check that all finals have been assigned can be made optional by an
explicit call to a method (FieldAccess.checkAllFinalsSet() for example).
If possible, I’d rather not have any additional methods exposed on FieldSetter,
other than the overloaded sets. Let see how this works if we keep it as minimal
as possible. I’m going to take a run over all readObjects in the base module
that use unsafe or reflection, and rewrite to use this API.
-Chris.
Regards, Peter
