Fix zone in which temp-zone parsed data is allocated for the function scope on the boundary.

Fix zone in which temp-zone parsed data is allocated for the function scope on the boundary.

BUG=chromium:417697
Committed: https://crrev.com/c4ccbaa3ea09490098989b9b743bace954c7b369
Cr-Commit-Position: refs/heads/master@{#41271}

[Toon Verwaest, 2016-11-23 15:09:08]

verwaest@chromium.org changed reviewers:
+ marja@chromium.org

[Toon Verwaest, 2016-11-23 15:09:09]

ptal

[marja, 2016-11-23 18:52:56]

Cool! LAGTM (looks almost good to me)

The hashmap thing is my only non-trivial comment.

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1267: // Prepare scope for outer in the outer zone.
I can't parse this comment...

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1521: continue;
Here you terminate the recursion earlier than before; we should also check that
there are no inner scopes.

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1710: if (is_declaration_scope() &&
AsDeclarationScope()->is_lazily_parsed()) {
This could use a comment explaining why we do this... so, this is correct
because the scope contains no variable declarations, right? We could also DCHECK
that. And that there are no inner scopes (again).

https://codereview.chromium.org/2522223002/diff/40001/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2522223002/diff/40001/src/base/hashmap.h#newc...
src/base/hashmap.h:77: map_++;
This is quite a... nonobvious way for doing this. :/

So when iterated, the map will look empty, right?

Won't TemplateHashMapImpl dtor cause a problem with this?

https://codereview.chromium.org/2522223002/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2522223002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2654: // This Scope lives in the main zone. We'll migrate
data into that zone
This comment needs updating.

[Toon Verwaest, 2016-11-23 19:20:48]

Will deal with it tomorrow, thanks for the review

https://codereview.chromium.org/2522223002/diff/40001/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2522223002/diff/40001/src/base/hashmap.h#newc...
src/base/hashmap.h:77: map_++;
On 2016/11/23 18:52:56, marja wrote:
> This is quite a... nonobvious way for doing this. :/
> 
> So when iterated, the map will look empty, right?
> 
> Won't TemplateHashMapImpl dtor cause a problem with this?

Yeah it's because of the Start implementation... We're never call the dtor
though...

https://codereview.chromium.org/2522223002/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2522223002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2654: // This Scope lives in the main zone. We'll migrate
data into that zone
On 2016/11/23 18:52:56, marja wrote:
> This comment needs updating.

Actually it's still accurate...

[Toon Verwaest, 2016-11-24 09:08:11]

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1267: // Prepare scope for outer in the outer zone.
On 2016/11/23 18:52:55, marja wrote:
> I can't parse this comment...

Done.

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1521: continue;
On 2016/11/23 18:52:56, marja wrote:
> Here you terminate the recursion earlier than before; we should also check
that
> there are no inner scopes.

Done.

https://codereview.chromium.org/2522223002/diff/40001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1710: if (is_declaration_scope() &&
AsDeclarationScope()->is_lazily_parsed()) {
On 2016/11/23 18:52:55, marja wrote:
> This could use a comment explaining why we do this... so, this is correct
> because the scope contains no variable declarations, right? We could also
DCHECK
> that. And that there are no inner scopes (again).

Done.

https://codereview.chromium.org/2522223002/diff/80001/src/ast/scopes.cc
File src/ast/scopes.cc (right):

https://codereview.chromium.org/2522223002/diff/80001/src/ast/scopes.cc#newco...
src/ast/scopes.cc:1715: DCHECK(variables_.occupancy() == 0);
didn't do DCHECK_EQ(0, ...); since occupancy is unsigned and 0 would be signed
and the compiler complains :)

[marja, 2016-11-24 12:48:16]

lgtm

https://codereview.chromium.org/2522223002/diff/40001/src/parsing/parser.cc
File src/parsing/parser.cc (right):

https://codereview.chromium.org/2522223002/diff/40001/src/parsing/parser.cc#n...
src/parsing/parser.cc:2654: // This Scope lives in the main zone. We'll migrate
data into that zone
On 2016/11/23 19:20:48, Toon Verwaest wrote:
> On 2016/11/23 18:52:56, marja wrote:
> > This comment needs updating.
> 
> Actually it's still accurate...

Ah, you're right, since it is "into that zone", not "into this scope".

https://codereview.chromium.org/2522223002/diff/100001/src/base/hashmap.h
File src/base/hashmap.h (right):

https://codereview.chromium.org/2522223002/diff/100001/src/base/hashmap.h#new...
src/base/hashmap.h:74: // support iteration though. For that reason we increment
{map_}.
this is no longer true; you don't increment

[Toon Verwaest, 2016-11-24 12:50:06]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-11-24 12:50:06]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2522223002/#ps120001
(title: "Update comment")

[commit-bot: I haz the power, 2016-11-24 12:50:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 13:10:29]

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

[commit-bot: I haz the power, 2016-11-24 13:10:30]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/16857)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Toon Verwaest, 2016-11-24 13:20:01]

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

[commit-bot: I haz the power, 2016-11-24 13:20:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 13:39:06]

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

[commit-bot: I haz the power, 2016-11-24 13:39:07]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18494)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Toon Verwaest, 2016-11-24 14:58:10]

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

[commit-bot: I haz the power, 2016-11-24 14:58:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-11-24 15:13:54]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-11-24 15:13:55]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2522223002/#ps220001
(title: "simplify and add comment")

[commit-bot: I haz the power, 2016-11-24 15:14:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Toon Verwaest, 2016-11-24 15:17:51]

Description was changed from

==========
Fix zone in which temp-zone parsed data is allocated for the function scope on
the boundary.

BUG=
==========

to

==========
Fix zone in which temp-zone parsed data is allocated for the function scope on
the boundary.

BUG=chromium:417697
==========

[Toon Verwaest, 2016-11-24 15:26:23]

The CQ bit was unchecked by verwaest@chromium.org

[Toon Verwaest, 2016-11-24 15:27:20]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-11-24 15:27:21]

The patchset sent to the CQ was uploaded after l-g-t-m from marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2522223002/#ps240001
(title: "Also free map_")

[commit-bot: I haz the power, 2016-11-24 15:27:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 16:06:44]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480001240767910,
"parent_rev": "a3e7b5f68135d2589295a4d91d5c0654b6b76989", "commit_rev":
"b519f9b6523929eeacc6e05b1d26a4c8d2b56ea4"}

[commit-bot: I haz the power, 2016-11-24 16:06:48]

Description was changed from

==========
Fix zone in which temp-zone parsed data is allocated for the function scope on
the boundary.

BUG=chromium:417697
==========

to

==========
Fix zone in which temp-zone parsed data is allocated for the function scope on
the boundary.

BUG=chromium:417697
==========

[commit-bot: I haz the power, 2016-11-24 16:06:49]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-11-24 16:07:13]

Description was changed from

==========
Fix zone in which temp-zone parsed data is allocated for the function scope on
the boundary.

BUG=chromium:417697
==========

to

==========
Fix zone in which temp-zone parsed data is allocated for the function scope on
the boundary.

BUG=chromium:417697

Committed: https://crrev.com/c4ccbaa3ea09490098989b9b743bace954c7b369
Cr-Commit-Position: refs/heads/master@{#41271}
==========

[commit-bot: I haz the power, 2016-11-24 16:07:14]

Patchset 13 (id:??) landed as
https://crrev.com/c4ccbaa3ea09490098989b9b743bace954c7b369
Cr-Commit-Position: refs/heads/master@{#41271}