https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/full-codegen-arm.cc
File src/full-codegen/arm/full-codegen-arm.cc (right):
https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/full-codegen-arm.cc#newcode1471
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-codegen.cc
File src/full-codegen/full-codegen.cc (right):
https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/full-codegen.cc#newcode1600
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-codegen.cc#newcode1618
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-codegen.cc#newcode1623
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-codegen.cc#newcode1624
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-codegen.cc#newcode1628
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-codegen.cc#newcode1632
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_".
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
