Make JSFunction::BodyDescriptor the only single place that knows how to iterate JSFunction's body.

Make JSFunction::BodyDescriptor the only single place that knows how to iterate JSFunction's body.

The body descriptor supports different visiting policies: it could visit or skip
the code entry and it could visit or skip next function field.

BUG=v8:4531
LOG=Y

Committed: https://crrev.com/8e09ee1dba6ffc204d1300a72d486751ab26a128
Cr-Commit-Position: refs/heads/master@{#31915}

[Igor Sheludko, 2015-11-05 18:36:30]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2015-11-05 18:37:13]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-05 18:37:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422773007/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422773007/20001

[Igor Sheludko, 2015-11-05 18:37:45]

ishell@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Igor Sheludko, 2015-11-05 18:37:46]

PTAL

[Igor Sheludko, 2015-11-05 18:38:28]

Description was changed from

==========
Make JSFunction::BodyDescriptor the only single place that knows how to iterate
JSFunctions.

The body descriptor supports different visiting policies: it could visit or skip
the code entry and it could visit or skip next function field.

BUG=v8:4531
LOG=Y
==========

to

==========
Make JSFunction::BodyDescriptor the only single place that knows how to iterate
JSFunction's body.

The body descriptor supports different visiting policies: it could visit or skip
the code entry and it could visit or skip next function field.

BUG=v8:4531
LOG=Y
==========

[commit-bot: I haz the power, 2015-11-05 19:14:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-05 19:14:36]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2015-11-10 10:40:32]

Looking good, just nits.

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
src/heap/objects-visiting-inl.h:493: typedef FlexibleBodyVisitor<StaticVisitor,
I would vote for keeping delegator methods as mentioned in the comment in the
header file.

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (left):

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
src/heap/objects-visiting.h:408: static void VisitJSFunctionStrongCode(Heap*
heap, HeapObject* object);
Can we keep these two methods for readability and make them just dispatch to the
BodyDescriptor in question? Basically one-line delegate functions.

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (right):

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
src/heap/objects-visiting.h:255: // visit the code entry, compiler wants it to
be accessible, therefore public.
nit: Let's drop the "therefore public" part of the sentence. The weird thing is
not the method being public, it's that the method needs to exist in the first
place. The VisitCodeEntry is always public.

https://codereview.chromium.org/1422773007/diff/20001/src/profiler/heap-snaps...
File src/profiler/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/1422773007/diff/20001/src/profiler/heap-snaps...
src/profiler/heap-snapshot-generator.cc:1161: // Ensure there are no new weak
references appeared in JSFunction.
nit: Drop the "there are" in this sentence.

[Igor Sheludko, 2015-11-10 10:58:11]

Thanks!

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
src/heap/objects-visiting-inl.h:493: typedef FlexibleBodyVisitor<StaticVisitor,
On 2015/11/10 10:40:32, Michael Starzinger wrote:
> I would vote for keeping delegator methods as mentioned in the comment in the
> header file.

Done.

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (left):

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
src/heap/objects-visiting.h:408: static void VisitJSFunctionStrongCode(Heap*
heap, HeapObject* object);
On 2015/11/10 10:40:32, Michael Starzinger wrote:
> Can we keep these two methods for readability and make them just dispatch to
the
> BodyDescriptor in question? Basically one-line delegate functions.

Done.

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
File src/heap/objects-visiting.h (right):

https://codereview.chromium.org/1422773007/diff/20001/src/heap/objects-visiti...
src/heap/objects-visiting.h:255: // visit the code entry, compiler wants it to
be accessible, therefore public.
On 2015/11/10 10:40:32, Michael Starzinger wrote:
> nit: Let's drop the "therefore public" part of the sentence. The weird thing
is
> not the method being public, it's that the method needs to exist in the first
> place. The VisitCodeEntry is always public.

Done.

https://codereview.chromium.org/1422773007/diff/20001/src/profiler/heap-snaps...
File src/profiler/heap-snapshot-generator.cc (right):

https://codereview.chromium.org/1422773007/diff/20001/src/profiler/heap-snaps...
src/profiler/heap-snapshot-generator.cc:1161: // Ensure there are no new weak
references appeared in JSFunction.
On 2015/11/10 10:40:32, Michael Starzinger wrote:
> nit: Drop the "there are" in this sentence.

Done.

[Igor Sheludko, 2015-11-10 11:02:03]

The CQ bit was checked by ishell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 11:02:12]

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

[commit-bot: I haz the power, 2015-11-10 11:38:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 11:38:41]

Dry run: This issue passed the CQ dry run.

[Igor Sheludko, 2015-11-10 11:39:06]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2015-11-10 11:39:11]

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

[commit-bot: I haz the power, 2015-11-10 11:39:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 11:39:12]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Igor Sheludko, 2015-11-10 11:42:50]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2015-11-10 11:43:06]

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

[commit-bot: I haz the power, 2015-11-10 11:43:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 11:43:10]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Michael Starzinger, 2015-11-10 11:44:19]

LGTM.

[Igor Sheludko, 2015-11-10 11:44:40]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2015-11-10 11:44:45]

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

[commit-bot: I haz the power, 2015-11-10 11:46:18]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-11-10 11:46:47]

Patchset 2 (id:??) landed as
https://crrev.com/8e09ee1dba6ffc204d1300a72d486751ab26a128
Cr-Commit-Position: refs/heads/master@{#31915}