https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2982
src/parser.cc:2982: factory()->NewBlock(labels, 2, true,
RelocInfo::kNoPosition);
On 2015/08/21 at 01:19:08, adamk wrote:
Seems weird that you pass labels both here and to the switch
statement. Are they both needed? Given that all the blocks end at the
same place, it seems like you should only need the labels (and the
target) in one place. Probably on the switch?
Can you add some tests with labeled breaks inside switch, with labels
at various places (on a block containing a switch, on the switch itself,
on a block contained in the switch)?
Done
https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2983
src/parser.cc:2983: Scope* switch_scope = NewScope(scope_, BLOCK_SCOPE);
On 2015/08/21 at 01:19:08, adamk wrote:
As discussed, I don't think you need this scope (and in fact it'll
already be dropped for you). It's fine to have a block without a scope.
Removed it.
https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode2988
src/parser.cc:2988: // This target exists in case the tag has a
do-expression
On 2015/08/21 at 01:19:08, adamk wrote:
This comment likely goes away if you simplify the way labels/targets
are used (and it should go away, as do-expressions are not yet a thing)
Done
https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3010
src/parser.cc:3010: factory()->NewSwitchStatement(labels, switch_pos);
On 2015/08/21 at 01:19:07, adamk wrote:
See above, I'd drop the labels here (pass NULL).
Passed the labels here, as you suggested above.
https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3021
src/parser.cc:3021: cases = new (zone()) ZoneList<CaseClause*>(4,
zone());
On 2015/08/21 at 01:19:08, adamk wrote:
Can you move this initialization up to where cases is declared? Seems
like that'd be simpler.
Moved the declaration down to here.
https://codereview.chromium.org/1293283002/diff/100001/src/parser.cc#newcode3034
src/parser.cc:3034: Expect(Token::RBRACE, CHECK_OK);
On 2015/08/21 at 01:19:08, adamk wrote:
I think this should move above the scope stuff just above, the closing
brace is effectively the end of the scope.
Can you update the comment about end positions in scopes.h to mention
switch while you're at it?
Done
https://codereview.chromium.org/1293283002/
--
--
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.
