Ensure hole checks take place in switch statement scopes

Ensure hole checks take place in switch statement scopes

Switch statements introduce their own scope for cases, but this scope
is not necessarily executed in order, as the following function shows:

  switch (x) {
    case 1:
      let y = 1;
    case 2:
      y = 2;
    case 3:
      print(y);
  }

If x = 2 or x = 3, the code should throw a ReferenceError. However,
FullCodeGen's hole check elimination used the simple algorithm of
assuming that if the initializer was in the same scope, then it was
reached before the use, and therefore the hole check could be
eliminated.

This patch adds an extra bit to scopes, to track if they may
nonlinearly. The parser marks the scope that switch introduces as
nonlinear. FullCodeGen does not eliminate the hole check from
a scope which is nonlinear. This patch refactors FullCodeGen to
put the hole check elimination in one place, rather than in each
backend.

BUG=v8:3926
LOG=Y
R=adamk

Committed: https://crrev.com/d6fb6de7097da908fd0a66804027ea189b559c0f
Cr-Commit-Position: refs/heads/master@{#30453}

[Dan Ehrenberg, 2015-08-22 00:41:13]

littledan@chromium.org changed reviewers:
+ adamk@chromium.org

[Dan Ehrenberg, 2015-08-22 00:41:20]

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

[commit-bot: I haz the power, 2015-08-22 00:41:32]

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

[commit-bot: I haz the power, 2015-08-22 00:51:53]

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

[commit-bot: I haz the power, 2015-08-22 00:51:54]

Dry run: Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/8954)

[Dan Ehrenberg, 2015-08-24 21:26:15]

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

[commit-bot: I haz the power, 2015-08-24 21:26:21]

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

[Dan Ehrenberg, 2015-08-24 21:44:43]

The test still fails on TurboFan; I'm investigating.

[commit-bot: I haz the power, 2015-08-24 21:51:45]

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

[commit-bot: I haz the power, 2015-08-24 21:51:46]

Dry run: This issue passed the CQ dry run.

[adamk, 2015-08-24 22:15:01]

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:1471: // Uninitalized const bindings
outside of harmony mode are unholed.
Typo (pre-existing, but might as well fix now): s/Uninitalized/Uninitialized/

That'll take you over 80 chars, but you could change this to:

// Uninitialized legacy const bindings are unholed.

Same goes for all other full codegen files.

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1600: DCHECK(var->scope() != NULL);
Can you also add a DCHECK about the location? Now that this code doesn't live
directly in a switch statement, that'd help further-rationalize this comment:

DCHECK(var->location() == VariableLocation::PARAMETER ||
       var->location() == VariableLocation::LOCAL ||
       var->location() == VariableLocation::CONTEXT);

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1618: // The check cannot be skipped on
non-linear scopes, namely switch
I think the bit about non-linear scopes should be incorporated into the first
paragraph of this comment, as one of the requirements, just after the same-scope
requirement. Leaving this example here is fine, though.

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1623: } else if (var->is_this()) {
I'd just make this a separate if statement, no need for an 'else' after a
return.

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1624: CHECK(literal() != nullptr &&
This CHECK looks scary, any idea why it's here?

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1628: } else {
Same thing here, I'd remove the else.

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1632: return var->mode() == CONST_LEGACY ||
scope()->is_nonlinear() ||
I think you need to be checking var->scope()->is_nonlinear(). Otherwise the
following code would incorrectly pass this check (please add a test):

switch (1) {
  case 0:
    let x = 2;
  case 1:
  { // this block, plus the let below, adds another lexical scope, this one
linear
    let y = 3;
    x = 2;
  }
}

https://codereview.chromium.org/1312613003/diff/20001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1312613003/diff/20001/src/scopes.h#newcode227
src/scopes.h:227: // Inform the scope that the may execute declarations
nonlinearly.
Comment unclear...I think the "the" is either misplaced or is supposed to be
something else? I'd also mention switch explicitly here as an example.

Stepping back for a second, is there any other non-linear scope? I'm curious why
you went with the more general term instead of calling this "is_switch_scope_".

[Dan Ehrenberg, 2015-08-25 01:23:19]

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

[commit-bot: I haz the power, 2015-08-25 01:23:27]

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

[Dan Ehrenberg, 2015-08-25 01:48:14]

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/fu...
File src/full-codegen/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/fu...
src/full-codegen/arm/full-codegen-arm.cc:1471: // Uninitalized const bindings
outside of harmony mode are unholed.
On 2015/08/24 at 22:15:00, adamk wrote:
> Typo (pre-existing, but might as well fix now): s/Uninitalized/Uninitialized/
> 
> That'll take you over 80 chars, but you could change this to:
> 
> // Uninitialized legacy const bindings are unholed.
> 
> Same goes for all other full codegen files.

Fixed

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1600: DCHECK(var->scope() != NULL);
On 2015/08/24 at 22:15:01, adamk wrote:
> Can you also add a DCHECK about the location? Now that this code doesn't live
directly in a switch statement, that'd help further-rationalize this comment:
> 
> DCHECK(var->location() == VariableLocation::PARAMETER ||
>        var->location() == VariableLocation::LOCAL ||
>        var->location() == VariableLocation::CONTEXT);

Good idea, done

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1618: // The check cannot be skipped on
non-linear scopes, namely switch
On 2015/08/24 at 22:15:01, adamk wrote:
> I think the bit about non-linear scopes should be incorporated into the first
paragraph of this comment, as one of the requirements, just after the same-scope
requirement. Leaving this example here is fine, though.

Done

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1623: } else if (var->is_this()) {
On 2015/08/24 at 22:15:01, adamk wrote:
> I'd just make this a separate if statement, no need for an 'else' after a
return.

Done

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1624: CHECK(literal() != nullptr &&
On 2015/08/24 at 22:15:01, adamk wrote:
> This CHECK looks scary, any idea why it's here?

Not exactly. I guess when 'this' occurs globally, it's resolved more
dynamically, and when it's in a function which isn't a subclass constructor,
it's resolved more dynamically, but I'm having trouble finding that code. I made
this into a DCHECK.

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1628: } else {
On 2015/08/24 at 22:15:01, adamk wrote:
> Same thing here, I'd remove the else.

Done

https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1632: return var->mode() == CONST_LEGACY ||
scope()->is_nonlinear() ||
On 2015/08/24 at 22:15:01, adamk wrote:
> I think you need to be checking var->scope()->is_nonlinear(). Otherwise the
following code would incorrectly pass this check (please add a test):
> 
> switch (1) {
>   case 0:
>     let x = 2;
>   case 1:
>   { // this block, plus the let below, adds another lexical scope, this one
linear
>     let y = 3;
>     x = 2;
>   }
> }

Oh, good catch! I fixed the bug and added a test based on this case which failed
with the previous version.

https://codereview.chromium.org/1312613003/diff/20001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1312613003/diff/20001/src/scopes.h#newcode227
src/scopes.h:227: // Inform the scope that the may execute declarations
nonlinearly.
On 2015/08/24 at 22:15:01, adamk wrote:
> Comment unclear...I think the "the" is either misplaced or is supposed to be
something else? I'd also mention switch explicitly here as an example.
> 
> Stepping back for a second, is there any other non-linear scope? I'm curious
why you went with the more general term instead of calling this
"is_switch_scope_".

Explained in the code.

[commit-bot: I haz the power, 2015-08-25 07:23:39]

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

[commit-bot: I haz the power, 2015-08-25 07:23:40]

Dry run: Try jobs failed on following builders:
  v8_win_rel on tryserver.v8 (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/8979)

[adamk, 2015-08-25 19:13:33]

lgtm % nits

https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1601: DCHECK(var->location() ==
VariableLocation::PARAMETER ||
Can you move this up above the other one, to keep the logic ordering the same as
before?

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/mjsunit.st...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:158: # issue 3926:
This should be 4388

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-3926.js (right):

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-3926.js:7: // Switch statements should disable let
hole checking
I think you mean should disable "hole check elimination"

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-3926.js:32: { // this block, plus the let below,
adds another lexical scope, this one linear
Nit: 80 chars

[Dan Ehrenberg, 2015-08-25 20:47:36]

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

[Dan Ehrenberg, 2015-08-25 20:54:45]

https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1601: DCHECK(var->location() ==
VariableLocation::PARAMETER ||
On 2015/08/25 at 19:13:33, adamk wrote:
> Can you move this up above the other one, to keep the logic ordering the same
as before?

Done

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/mjsunit.st...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:158: # issue 3926:
On 2015/08/25 at 19:13:33, adamk wrote:
> This should be 4388

Done

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-3926.js (right):

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-3926.js:7: // Switch statements should disable let
hole checking
On 2015/08/25 at 19:13:33, adamk wrote:
> I think you mean should disable "hole check elimination"

Done

https://codereview.chromium.org/1312613003/diff/40001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-3926.js:32: { // this block, plus the let below,
adds another lexical scope, this one linear
On 2015/08/25 at 19:13:33, adamk wrote:
> Nit: 80 chars

Done

[Dan Ehrenberg, 2015-08-25 20:54:55]

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

[commit-bot: I haz the power, 2015-08-25 20:55:06]

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

[commit-bot: I haz the power, 2015-08-25 21:21:10]

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

[commit-bot: I haz the power, 2015-08-25 21:21:10]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2015-08-26 11:30:29]

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

[Michael Starzinger, 2015-08-26 11:30:29]

mstarzinger@chromium.org changed required reviewers:
+ rossberg@chromium.org

[Michael Starzinger, 2015-08-26 11:30:30]

LGTM on full codegen, just nits. But one concern about scope serialization,
adding Andreas for that.

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1588: bool
FullCodeGenerator::NeedsHoleCheck(VariableProxy* proxy) {
nit: Can we call this predicate either "NeedsHoleCheckOnLoad" or
"NeedsHoleCheckForLoad", otherwise people might get the idea to use this for
assignments at some point and become disappointed.

https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
Isn't it necessary to serialize this flag into the ScopeInfo? Adding Andreas
whose scoping foo is clearly superior to mine.

https://codereview.chromium.org/1312613003/diff/60001/test/mjsunit/mjsunit.st...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1312613003/diff/60001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:159: 'regress/regress-3926': [PASS, NO_VARIANTS],
This should no longer be needed after my fix for issue 4388 that already landed.

[Michael Starzinger, 2015-08-26 11:36:58]

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.h (right):

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.h:704: bool NeedsHoleCheck(VariableProxy* proxy);
nit: Please move the declaration away from the simple getters, anywhere else
than here is better, how about line 553 after EmitVariableLoad?

[rossberg, 2015-08-26 12:07:58]

https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
On 2015/08/26 11:30:30, Michael Starzinger wrote:
> Isn't it necessary to serialize this flag into the ScopeInfo? Adding Andreas
> whose scoping foo is clearly superior to mine.

Indeed, AFAICS this flag needs to be de/serialized in scopeinfos, otherwise
lazily compiled local functions inside switch blocks will still miss hole
checks. A test for that case would be good, too.

[rossberg, 2015-08-26 12:09:25]

On 2015/08/26 12:07:58, rossberg wrote:
> https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
> File src/scopes.h (right):
> 
> https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
> src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
> On 2015/08/26 11:30:30, Michael Starzinger wrote:
> > Isn't it necessary to serialize this flag into the ScopeInfo? Adding Andreas
> > whose scoping foo is clearly superior to mine.
> 
> Indeed, AFAICS this flag needs to be de/serialized in scopeinfos, otherwise
> lazily compiled local functions inside switch blocks will still miss hole
> checks. A test for that case would be good, too.

...and likewise in local evals, me thinks.

[Dan Ehrenberg, 2015-08-27 01:40:49]

Could we leave tracking nonlinear scopes in ScopeInfo for a future patch? This
one brings the world further towards correctness as-is. And I'm not sure how to
trigger a test case--everything I try just works.

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.h (right):

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.h:704: bool NeedsHoleCheck(VariableProxy* proxy);
On 2015/08/26 at 11:36:58, Michael Starzinger wrote:
> nit: Please move the declaration away from the simple getters, anywhere else
than here is better, how about line 553 after EmitVariableLoad?


Done

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.h:704: bool NeedsHoleCheck(VariableProxy* proxy);
On 2015/08/26 at 11:36:58, Michael Starzinger wrote:
> nit: Please move the declaration away from the simple getters, anywhere else
than here is better, how about line 553 after EmitVariableLoad?


Done

https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
File src/scopes.h (right):

https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
On 2015/08/26 at 12:07:58, rossberg wrote:
> On 2015/08/26 11:30:30, Michael Starzinger wrote:
> > Isn't it necessary to serialize this flag into the ScopeInfo? Adding Andreas
> > whose scoping foo is clearly superior to mine.
> 
> Indeed, AFAICS this flag needs to be de/serialized in scopeinfos, otherwise
lazily compiled local functions inside switch blocks will still miss hole
checks. A test for that case would be good, too.

Well, that issue won't hit fullcodegen, as hole checks are only eliminated
within a particular block scope. Would it hurt any other compiler? I can't
figure out how to trigger it in TF.

[Dan Ehrenberg, 2015-08-27 01:56:23]

On 2015/08/27 at 01:40:49, Dan Ehrenberg wrote:
> Could we leave tracking nonlinear scopes in ScopeInfo for a future patch? This
one brings the world further towards correctness as-is. And I'm not sure how to
trigger a test case--everything I try just works.
> 
>
https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
> File src/full-codegen/full-codegen.h (right):
> 
>
https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
> src/full-codegen/full-codegen.h:704: bool NeedsHoleCheck(VariableProxy*
proxy);
> On 2015/08/26 at 11:36:58, Michael Starzinger wrote:
> > nit: Please move the declaration away from the simple getters, anywhere else
than here is better, how about line 553 after EmitVariableLoad?
> 
> 
> Done
> 
>
https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
> src/full-codegen/full-codegen.h:704: bool NeedsHoleCheck(VariableProxy*
proxy);
> On 2015/08/26 at 11:36:58, Michael Starzinger wrote:
> > nit: Please move the declaration away from the simple getters, anywhere else
than here is better, how about line 553 after EmitVariableLoad?
> 
> 
> Done
> 
> https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
> File src/scopes.h (right):
> 
> https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
> src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
> On 2015/08/26 at 12:07:58, rossberg wrote:
> > On 2015/08/26 11:30:30, Michael Starzinger wrote:
> > > Isn't it necessary to serialize this flag into the ScopeInfo? Adding
Andreas
> > > whose scoping foo is clearly superior to mine.
> > 
> > Indeed, AFAICS this flag needs to be de/serialized in scopeinfos, otherwise
lazily compiled local functions inside switch blocks will still miss hole
checks. A test for that case would be good, too.
> 
> Well, that issue won't hit fullcodegen, as hole checks are only eliminated
within a particular block scope. Would it hurt any other compiler? I can't
figure out how to trigger it in TF.

It looks to me like Turbofan always does the hole checks on
VariableLocation::CONTEXT let variables (which should handle closure and eval
cases, right?) and Crankshaft doesn't do hole check elimination at all. I
mentioned above that fullcodegen is not a problem.

[adamk, 2015-08-27 16:21:04]

Your reasoning re: ScopeInfo sounds right to me. Can you add a note to scopes.h
explaining why the bit isn't needed on ScopeInfo?

Also, could you add a few more tests (the ones suggested by Andreas)? In
particular, tests for assignment and loading inside evals (both sloppy and
strict would be good for coverage) and tests for lazy local functions that
assign and load. That'll make sure your comment isn't the only thing keeping
those hole checks in place.

[Dan Ehrenberg, 2015-08-27 18:33:22]

On 2015/08/27 at 16:21:04, adamk wrote:
> Your reasoning re: ScopeInfo sounds right to me. Can you add a note to
scopes.h explaining why the bit isn't needed on ScopeInfo?
> 
> Also, could you add a few more tests (the ones suggested by Andreas)? In
particular, tests for assignment and loading inside evals (both sloppy and
strict would be good for coverage) and tests for lazy local functions that
assign and load. That'll make sure your comment isn't the only thing keeping
those hole checks in place.

Done

[adamk, 2015-08-27 18:42:07]

lgtm once you've added sloppy eval tests

[Michael Starzinger, 2015-08-27 19:10:09]

Just pinging on one of my comments so that it doesn't drown in the noise. Still
LGTM after that of course.

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (right):

https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:1588: bool
FullCodeGenerator::NeedsHoleCheck(VariableProxy* proxy) {
On 2015/08/26 11:30:29, Michael Starzinger wrote:
> nit: Can we call this predicate either "NeedsHoleCheckOnLoad" or
> "NeedsHoleCheckForLoad", otherwise people might get the idea to use this for
> assignments at some point and become disappointed.

Ping.

[adamk, 2015-08-27 19:12:49]

Final list of things that need to be fixed before landing:

- Renaming of NeedsHoleCheck method
- Test for sloppy eval

Also on the menu, but can be handled in a follow-up (since it's not related to
hole-checking): add tests for completion value of switch, and fix the desugaring
to handle that properly in case it's not already handled.

[Dan Ehrenberg, 2015-08-27 20:50:38]

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

[commit-bot: I haz the power, 2015-08-27 20:50:47]

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

[Dan Ehrenberg, 2015-08-27 20:59:50]

The CQ bit was unchecked by littledan@chromium.org

[Dan Ehrenberg, 2015-08-27 20:59:52]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2015-08-27 20:59:52]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1312613003/#ps120001
(title: "Rename function and add sloppy mode tests")

[commit-bot: I haz the power, 2015-08-27 20:59:57]

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

[commit-bot: I haz the power, 2015-08-27 21:00:01]

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

[commit-bot: I haz the power, 2015-08-27 21:00:02]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

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.

[adamk, 2015-08-28 00:15:49]

adamk@chromium.org changed required reviewers:
- rossberg@chromium.org

[adamk, 2015-08-28 00:15:50]

lgtm, I think you've satisfied rossberg's requirements with the added tests and
comment in scopes.h

[Dan Ehrenberg, 2015-08-28 17:18:31]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2015-08-28 17:18:38]

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

[commit-bot: I haz the power, 2015-08-28 18:49:52]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-08-28 18:50:15]

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