[interpreter] Make setting of function data more resilient.

[interpreter] Make setting of function data more resilient.

This adds explicit setters for the SharedFunctionInfo::function_data
field. Such setters are safer because they allow for explicit checking
of which values are allowed, and they improve readability because the
intended semantics become clear for each call-site. Also fix a cctest
case along the way.

R=rmcilroy@chromium.org

Committed: https://crrev.com/6acee6ee598f4638e1ee376d51243821787acb3e
Cr-Commit-Position: refs/heads/master@{#34297}

[Michael Starzinger, 2016-02-25 13:46:03]

Description was changed from

==========
[interpreter] Make setting of function data more resilient.

This adds explicit setters for the SharedFunctionInfo::function_data
field. Such setters are safer because they allow for explicit checking
of which values are allowed, and they improve readability because the
intended semantics become clear for each callsite. Also fix a cctest
case along the way.

R=rmcilroy@chromium.org
==========

to

==========
[interpreter] Make setting of function data more resilient.

This adds explicit setters for the SharedFunctionInfo::function_data
field. Such setters are safer because they allow for explicit checking
of which values are allowed, and they improve readability because the
intended semantics become clear for each call-site. Also fix a cctest
case along the way.

R=rmcilroy@chromium.org
==========

[rmcilroy, 2016-02-25 14:18:06]

LGTM with some comments.

https://codereview.chromium.org/1730853005/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1730853005/diff/1/src/objects-inl.h#newcode5851
src/objects-inl.h:5851: DCHECK(bytecode->IsBytecodeArray() ||
bytecode->IsUndefined());
Is this ever set to undefined other than the test you changed below? Could we
instead add a ClearBytecodeArray() option instead?

https://codereview.chromium.org/1730853005/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1730853005/diff/1/src/objects.h#newcode6727
src/objects.h:6727: DECL_ACCESSORS(function_data, Object)
Can we make this accessor private now?

[Michael Starzinger, 2016-02-25 14:43:44]

PTAL. Addressed one out of two comments.

https://codereview.chromium.org/1730853005/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1730853005/diff/1/src/objects-inl.h#newcode5851
src/objects-inl.h:5851: DCHECK(bytecode->IsBytecodeArray() ||
bytecode->IsUndefined());
On 2016/02/25 14:18:06, rmcilroy wrote:
> Is this ever set to undefined other than the test you changed below? Could we
> instead add a ClearBytecodeArray() option instead?

Done. Only done in the test. Added the ClearBytecodeArray() method as suggested,
good idea!

https://codereview.chromium.org/1730853005/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1730853005/diff/1/src/objects.h#newcode6727
src/objects.h:6727: DECL_ACCESSORS(function_data, Object)
On 2016/02/25 14:18:06, rmcilroy wrote:
> Can we make this accessor private now?

Acknowledged. Unfortunately not, heap still needs it for the initializing store.

https://codereview.chromium.org/1730853005/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1730853005/diff/40001/src/objects-inl.h#newco...
src/objects-inl.h:5851: DCHECK(function_data()->IsUndefined());
This would now imply the DCHECKs in compiler.cc about only compiling once.
However I am inclined to leave the explicit DCHECKs in compiler.cc as
"documentation". Please let me know if you disagree.

[rmcilroy, 2016-02-25 15:33:56]

LGTM, thanks

https://codereview.chromium.org/1730853005/diff/40001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1730853005/diff/40001/src/objects-inl.h#newco...
src/objects-inl.h:5851: DCHECK(function_data()->IsUndefined());
On 2016/02/25 14:43:43, Michael Starzinger wrote:
> This would now imply the DCHECKs in compiler.cc about only compiling once.
> However I am inclined to leave the explicit DCHECKs in compiler.cc as
> "documentation". Please let me know if you disagree.

SGTM

[Michael Starzinger, 2016-02-25 15:36:44]

The CQ bit was checked by mstarzinger@chromium.org

[commit-bot: I haz the power, 2016-02-25 15:36:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1730853005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1730853005/40001

[commit-bot: I haz the power, 2016-02-25 16:06:08]

Description was changed from

==========
[interpreter] Make setting of function data more resilient.

This adds explicit setters for the SharedFunctionInfo::function_data
field. Such setters are safer because they allow for explicit checking
of which values are allowed, and they improve readability because the
intended semantics become clear for each call-site. Also fix a cctest
case along the way.

R=rmcilroy@chromium.org
==========

to

==========
[interpreter] Make setting of function data more resilient.

This adds explicit setters for the SharedFunctionInfo::function_data
field. Such setters are safer because they allow for explicit checking
of which values are allowed, and they improve readability because the
intended semantics become clear for each call-site. Also fix a cctest
case along the way.

R=rmcilroy@chromium.org
==========

[commit-bot: I haz the power, 2016-02-25 16:06:10]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-25 16:07:29]

Description was changed from

==========
[interpreter] Make setting of function data more resilient.

This adds explicit setters for the SharedFunctionInfo::function_data
field. Such setters are safer because they allow for explicit checking
of which values are allowed, and they improve readability because the
intended semantics become clear for each call-site. Also fix a cctest
case along the way.

R=rmcilroy@chromium.org
==========

to

==========
[interpreter] Make setting of function data more resilient.

This adds explicit setters for the SharedFunctionInfo::function_data
field. Such setters are safer because they allow for explicit checking
of which values are allowed, and they improve readability because the
intended semantics become clear for each call-site. Also fix a cctest
case along the way.

R=rmcilroy@chromium.org

Committed: https://crrev.com/6acee6ee598f4638e1ee376d51243821787acb3e
Cr-Commit-Position: refs/heads/master@{#34297}
==========

[commit-bot: I haz the power, 2016-02-25 16:07:30]

Patchset 3 (id:??) landed as
https://crrev.com/6acee6ee598f4638e1ee376d51243821787acb3e
Cr-Commit-Position: refs/heads/master@{#34297}