Rename accessors of class Field to make it more apparent as to what is being accessed - static fiel…

Changes to prepare for allowing script snapshots to be taken after running the main application:

- Rename accessors of class Field to make it more apparent as to what is being accessed (static field values or instance field offsets
- Use precompiled initializer state (Used only during precompilation) to also store saved initial value of static fields (Used only during application snapshot   generation)

BUG=
R=hausner@google.com, rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/79ef28317f767934db88b27f86b2b76934d46ed6

[siva, 2015-09-01 01:11:28]

asiva@google.com changed reviewers:
+ hausner@google.com, rmacnak@google.com

[hausner, 2015-09-01 16:02:02]

Technically LGTM, but I have some reservations about the renaming of the
accessors and raw fields. I think the code becomes less readable, because the
names are too long. E.g. StaticFieldValue() is pretty much exclusively used on a
variable named field, so you have the the substring 'field' in the expression
twice, which is redundant. The expressions become long and have to be split over
multiple lines, which doesn't help readability.

The code looks more and more like it's written by Welsh people from the town of
Llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:3807: ASSERT(node->field().StaticFieldValue()
!=
field().StaticFieldValue() seems a bit redundant, with the word Field contained
twice.  I didn't have a problem with field().value().

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:832: union {
Seems a bit over the top to me:

field->value_.static_value_
field->value_.offset_
field->value_.value_

I'm not sure this is more readable than what we have now. Also, only the
accessor functions touch value_, and you already add more specific names to
those.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:840: RawInstance* saved_initial_value_;  // Saved
initial value (static fields).
The comment just repeats what the identifier already says. Maybe a longer
comment explaining a bigger picture how/why initializer_ is used would be more
helpful.

[rmacnak, 2015-09-01 16:54:56]

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/object.h#new...
runtime/vm/object.h:2828: RawInstance* StaticFieldValue() const;
FWIW, the service protocol simply calls this "staticValue".

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:816: // Write out the name.
WriteObjectImpl doesn't care about the C++ type of the object it is writing, so
I don't think this change is needed.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:824:
writer->WriteObjectImpl(ptr()->initializer_.value_, kAsReference);
value_.value_

[rmacnak, 2015-09-01 18:28:51]

lgtm

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:824:
writer->WriteObjectImpl(ptr()->initializer_.value_, kAsReference);
On 2015/09/01 16:54:55, rmacnak wrote:
> value_.value_

Okay, nvm. Add a comment in the then branch that we are overwriting the
staticValue with the saved_initial_value.

[siva, 2015-09-03 23:32:10]

I have addressed some of the long names concerns.
I have left the union part (removed the untyped access field), if you guys feel
very strongly about it I can reconsider and see if there is another way to
nicely document the field overloads.

PTAL.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:3807: ASSERT(node->field().StaticFieldValue()
!=
On 2015/09/01 16:02:02, hausner wrote:
> field().StaticFieldValue() seems a bit redundant, with the word Field
contained
> twice.  I didn't have a problem with field().value().


I have renamed this to StaticValue as suggested by Ryan.
I feel field().StaticValue() is more readable that field().value() which appears
to be like a regular accessor that we have for fields and  it could return
whatever is in value_ (offset or static value).

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/object.h#new...
runtime/vm/object.h:2828: RawInstance* StaticFieldValue() const;
On 2015/09/01 16:54:55, rmacnak wrote:
> FWIW, the service protocol simply calls this "staticValue".

Acknowledged.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:832: union {
On 2015/09/01 16:02:02, hausner wrote:
> Seems a bit over the top to me:
> 
> field->value_.static_value_
> field->value_.offset_
> field->value_.value_
> 
> I'm not sure this is more readable than what we have now. Also, only the
> accessor functions touch value_, and you already add more specific names to
> those.

My main motivation for doing this was to have a way of documenting what the
overloads are without having to hunt through all the different accessors in
object.h.

I checked this out with a few people and they seemed to be open about this, if
you feel very strongly about this I can leave this alone and only work with the
accessors and maybe think of a better way to document the overloads.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:840: RawInstance* saved_initial_value_;  // Saved
initial value (static fields).
On 2015/09/01 16:02:02, hausner wrote:
> The comment just repeats what the identifier already says. Maybe a longer
> comment explaining a bigger picture how/why initializer_ is used would be more
> helpful.

Done.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:816: // Write out the name.
On 2015/09/01 16:54:55, rmacnak wrote:
> WriteObjectImpl doesn't care about the C++ type of the object it is writing,
so
> I don't think this change is needed.

As discussed offline this is used to ensure that we write out the saved initial
value for static fields. I have added a comment to make that clear.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:824:
writer->WriteObjectImpl(ptr()->initializer_.value_, kAsReference);
On 2015/09/01 18:28:51, rmacnak wrote:
> On 2015/09/01 16:54:55, rmacnak wrote:
> > value_.value_
> 
> Okay, nvm. Add a comment in the then branch that we are overwriting the
> staticValue with the saved_initial_value.

Acknowledged.

https://codereview.chromium.org/1289643005/diff/60001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:824:
writer->WriteObjectImpl(ptr()->initializer_.value_, kAsReference);
On 2015/09/01 16:54:55, rmacnak wrote:
> value_.value_

initializer_.saved_value_;

[rmacnak, 2015-09-03 23:47:51]

lgtm

[hausner, 2015-09-04 16:46:26]

Thanks for making the names a bit shorter. LGTM too.

[siva, 2015-09-04 18:03:51]

Committed patchset #7 (id:120001) manually as
79ef28317f767934db88b27f86b2b76934d46ed6 (presubmit successful).