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.
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-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);
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-codegen.cc#newcode1618
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-codegen.cc#newcode1623
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-codegen.cc#newcode1624
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-codegen.cc#newcode1628
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-codegen.cc#newcode1632
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.
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
