Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(79)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: src/parser.cc

Issue 720863002: Fix desugaring of let bindings in for loops to handle continue properly (Closed) Base URL: https://v8.googlecode.com/svn/branches/bleeding_edge
Patch Set: New desugaring comment, not yet implemented Created 6 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « src/parser.h ('k') | test/mjsunit/harmony/regress/regress-3683.js » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/parser.cc
diff --git a/src/parser.cc b/src/parser.cc
index 7fc5dd0512b4ba73be38148b35a9faaffc63be8a..81044683fefcca4be5b23b502761d5e0538881b4 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -2983,7 +2983,7 @@ void Parser::InitializeForEachStatement(ForEachStatement* stmt,
Statement* Parser::DesugarLetBindingsInForStatement(
Scope* inner_scope, ZoneList<const AstRawString*>* names,
- ForStatement* loop, Statement* init, Expression* cond, Statement* next,
+ ForStatement* loop, Statement* init, Expression* cond, Expression* next,
Statement* body, bool* ok) {
// ES6 13.6.3.4 specifies that on each loop iteration the let variables are
// copied into a new environment. After copying, the "next" statement of the
@@ -2992,37 +2992,42 @@ Statement* Parser::DesugarLetBindingsInForStatement(
//
// We rewrite a for statement of the form
//
- // for (let x = i; cond; next) body
+ // labels: for (let x = i; cond; next) body
//
// into
//
// {
- // let x = i;
- // temp_x = x;
- // flag = 1;
- // for (;;) {
- // let x = temp_x;
- // if (flag == 1) {
- // flag = 0;
- // } else {
- // next;
- // }
+ // let x = i;
+ // temp_x = x;
+ // first = 1;
+ // outer: for (;;) {
+ // let x = temp_x;
+ // if (first == 1) {
+ // first = 0;
+ // } else {
+ // next;
+ // }
+ // flag = 1;
+ // labels: for (; flag == 1; temp_x = x, flag = 0) {
// if (cond) {
- // <empty>
+ // body
// } else {
- // break;
+ // break outer;
// }
- // b
- // temp_x = x;
- // }
+ // }
+ // if (flag == 1) {
+ // break;
+ // }
+ // }
// }
DCHECK(names->length() > 0);
Scope* for_scope = scope_;
ZoneList<Variable*> temps(names->length(), zone());
- Block* outer_block = factory()->NewBlock(NULL, names->length() + 3, false,
+ Block* outer_block = factory()->NewBlock(NULL, names->length() + 2, false,
RelocInfo::kNoPosition);
+
outer_block->AddStatement(init, zone());
const AstRawString* temp_name = ast_value_factory()->dot_for_string();
@@ -3042,24 +3047,20 @@ Statement* Parser::DesugarLetBindingsInForStatement(
temps.Add(temp, zone());
}
- Variable* flag = scope_->DeclarationScope()->NewTemporary(temp_name);
- // Make statement: flag = 1.
- {
- VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
- Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
- Assignment* assignment = factory()->NewAssignment(
- Token::ASSIGN, flag_proxy, const1, RelocInfo::kNoPosition);
- Statement* assignment_statement = factory()->NewExpressionStatement(
- assignment, RelocInfo::kNoPosition);
- outer_block->AddStatement(assignment_statement, zone());
- }
+ // Make statement: outer: for (;;)
+ // Note that we don't actually create the label, or set this loop up as an
+ // explicit break target, instead handing it directly to those nodes that
+ // need to know about it. This should be safe because we don't run any code
+ // in this function that looks up break targets.
+ ForStatement* outer_loop =
+ factory()->NewForStatement(NULL, RelocInfo::kNoPosition);
+ outer_block->AddStatement(outer_loop, zone());
- outer_block->AddStatement(loop, zone());
outer_block->set_scope(for_scope);
scope_ = inner_scope;
- Block* inner_block = factory()->NewBlock(NULL, 2 * names->length() + 3,
- false, RelocInfo::kNoPosition);
+ Block* inner_block = factory()->NewBlock(NULL, names->length() + 3, false,
+ RelocInfo::kNoPosition);
int pos = scanner()->location().beg_pos;
ZoneList<Variable*> inner_vars(names->length(), zone());
@@ -3081,61 +3082,98 @@ Statement* Parser::DesugarLetBindingsInForStatement(
inner_block->AddStatement(assignment_statement, zone());
}
- // Make statement: if (flag == 1) { flag = 0; } else { next; }.
- if (next) {
- Expression* compare = NULL;
- // Make compare expresion: flag == 1.
- {
- Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
- VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
- compare = factory()->NewCompareOperation(
- Token::EQ, flag_proxy, const1, pos);
+ Variable* flag = scope_->DeclarationScope()->NewTemporary(temp_name);
+ // Make statement: flag = 1.
+ {
+ VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
+ Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
arv (Not doing code reviews) 2014/11/13 16:15:43 Maybe we should make these default to RelocInfo::k
+ Assignment* assignment = factory()->NewAssignment(
+ Token::ASSIGN, flag_proxy, const1, RelocInfo::kNoPosition);
+ Statement* assignment_statement =
+ factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition);
+ inner_block->AddStatement(assignment_statement, zone());
+ }
+
+ // Make cond expression for main loop: flag == 1.
+ Expression* flag_cond = NULL;
+ {
+ Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
+ VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
+ flag_cond = factory()->NewCompareOperation(Token::EQ, flag_proxy, const1,
+ RelocInfo::kNoPosition);
+ }
+
+ // Create chain of expressions "next, temp_x = x, ..., flag = 0"
+ Statement* compound_next_statement = NULL;
+ {
+ Expression* compound_next = next;
+
+ // Make the comma-separated list of temp_x = x assignments.
+ for (int i = 0; i < names->length(); i++) {
+ VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i));
+ VariableProxy* proxy =
+ factory()->NewVariableProxy(inner_vars.at(i), RelocInfo::kNoPosition);
+ Assignment* assignment = factory()->NewAssignment(
+ Token::ASSIGN, temp_proxy, proxy, RelocInfo::kNoPosition);
+ if (compound_next) {
+ compound_next = factory()->NewBinaryOperation(
+ Token::COMMA, compound_next, assignment, RelocInfo::kNoPosition);
+ } else {
+ compound_next = assignment;
+ }
}
- Statement* clear_flag = NULL;
- // Make statement: flag = 0.
+
+ // Make expression: flag = 0.
{
VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
Expression* const0 = factory()->NewSmiLiteral(0, RelocInfo::kNoPosition);
Assignment* assignment = factory()->NewAssignment(
Token::ASSIGN, flag_proxy, const0, RelocInfo::kNoPosition);
- clear_flag = factory()->NewExpressionStatement(assignment, pos);
+ compound_next = factory()->NewBinaryOperation(
+ Token::COMMA, compound_next, assignment, RelocInfo::kNoPosition);
}
- Statement* clear_flag_or_next = factory()->NewIfStatement(
- compare, clear_flag, next, RelocInfo::kNoPosition);
- inner_block->AddStatement(clear_flag_or_next, zone());
- }
+ compound_next_statement = factory()->NewExpressionStatement(
+ compound_next, next ? next->position() : RelocInfo::kNoPosition);
+ }
- // Make statement: if (cond) { } else { break; }.
- if (cond) {
- Statement* empty = factory()->NewEmptyStatement(RelocInfo::kNoPosition);
- BreakableStatement* t = LookupBreakTarget(NULL, CHECK_OK);
- Statement* stop = factory()->NewBreakStatement(t, RelocInfo::kNoPosition);
- Statement* if_not_cond_break = factory()->NewIfStatement(
- cond, empty, stop, cond->position());
- inner_block->AddStatement(if_not_cond_break, zone());
+ // Make statement: if (cond) { body; } else { break aux; }
+ Statement* if_cond_body_else_stop = NULL;
+ {
+ Statement* stop =
+ factory()->NewBreakStatement(outer_loop, RelocInfo::kNoPosition);
+ if_cond_body_else_stop =
+ factory()->NewIfStatement(cond, body, stop, cond->position());
}
- inner_block->AddStatement(body, zone());
+ // TODO(adamk): Does the order matter?
+ loop->Initialize(NULL, flag_cond, compound_next_statement,
+ if_cond_body_else_stop);
+ inner_block->AddStatement(loop, zone());
- // For each let variable x:
- // make statement: temp_x = x;
- for (int i = 0; i < names->length(); i++) {
- VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i));
- int pos = scanner()->location().end_pos;
- VariableProxy* proxy = factory()->NewVariableProxy(inner_vars.at(i), pos);
- Assignment* assignment = factory()->NewAssignment(
- Token::ASSIGN, temp_proxy, proxy, RelocInfo::kNoPosition);
- Statement* assignment_statement = factory()->NewExpressionStatement(
- assignment, RelocInfo::kNoPosition);
- inner_block->AddStatement(assignment_statement, zone());
+ // Make statement: if (flag == 1) { break; }
+ {
+ Statement* empty = factory()->NewEmptyStatement(RelocInfo::kNoPosition);
+ Expression* compare = NULL;
+ // Make compare expresion: flag == 1.
+ {
+ Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition);
+ VariableProxy* flag_proxy = factory()->NewVariableProxy(flag);
+ compare =
+ factory()->NewCompareOperation(Token::EQ, flag_proxy, const1, pos);
+ }
+ Statement* stop =
+ factory()->NewBreakStatement(outer_loop, RelocInfo::kNoPosition);
+ Statement* if_flag_break =
+ factory()->NewIfStatement(compare, stop, empty, RelocInfo::kNoPosition);
+ inner_block->AddStatement(if_flag_break, zone());
}
inner_scope->set_end_position(scanner()->location().end_pos);
inner_block->set_scope(inner_scope);
scope_ = for_scope;
- loop->Initialize(NULL, NULL, NULL, inner_block);
+ outer_loop->Initialize(NULL, NULL, NULL, inner_block);
return outer_block;
}
@@ -3317,11 +3355,9 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
}
Expect(Token::SEMICOLON, CHECK_OK);
- Statement* next = NULL;
+ Expression* next = NULL;
if (peek() != Token::RPAREN) {
- int next_pos = position();
- Expression* exp = ParseExpression(true, CHECK_OK);
- next = factory()->NewExpressionStatement(exp, next_pos);
+ next = ParseExpression(true, CHECK_OK);
}
Expect(Token::RPAREN, CHECK_OK);
@@ -3335,6 +3371,10 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
scope_ = saved_scope;
for_scope->set_end_position(scanner()->location().end_pos);
} else {
+ Statement* next_stmt = NULL;
+ if (next) {
+ next_stmt = factory()->NewExpressionStatement(next, next->position());
+ }
scope_ = saved_scope;
for_scope->set_end_position(scanner()->location().end_pos);
for_scope = for_scope->FinalizeBlockScope();
@@ -3354,10 +3394,10 @@ Statement* Parser::ParseForStatement(ZoneList<const AstRawString*>* labels,
block->AddStatement(init, zone());
block->AddStatement(loop, zone());
block->set_scope(for_scope);
- loop->Initialize(NULL, cond, next, body);
+ loop->Initialize(NULL, cond, next_stmt, body);
result = block;
} else {
- loop->Initialize(init, cond, next, body);
+ loop->Initialize(init, cond, next_stmt, body);
result = loop;
}
}
« no previous file with comments | « src/parser.h ('k') | test/mjsunit/harmony/regress/regress-3683.js » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698