Move ParseVariableDeclarations to ParserBase.

src/parsing/parser-base.h

Index: src/parsing/parser-base.h
diff --git a/src/parsing/parser-base.h b/src/parsing/parser-base.h
index 96b54b4fd5ea25cf65b2f7d3f75863b444b349ba..96045c68cd4f65d239b245f1f15e3892a1b9c1a9 100644
--- a/src/parsing/parser-base.h
+++ b/src/parsing/parser-base.h
@@ -214,6 +214,7 @@ class ParserBase {
   typedef typename Types::StatementList StatementListT;
   typedef typename v8::internal::ExpressionClassifier<Types>
       ExpressionClassifier;
+  typedef typename Types::Block BlockT;
 
   // All implementation-specific methods must be called through this.
   Impl* impl() { return static_cast<Impl*>(this); }
@@ -934,6 +935,12 @@ class ParserBase {
                             static_cast<const char*>(nullptr), error_type);
   }
 
+  void ReportMessageAt(Scanner::Location location,
+                       MessageTemplate::Template message, const char* arg,
+                       ParseErrorType error_type = kSyntaxError) {
+    impl()->ReportMessageAt(location, message, arg, error_type);
+  }

[nickie, 2016/09/01 10:53:08]

I suppose that by rearranging the parameters and putting "arg = nullptr" in the
end, it should be possible to merge this with the previous one.  But, for
consistency, we'd have to do the same in the impl()-> methods.  Then, it may
turn out that these two are not needed anymore and that we should be calling the
impl()-> methods directly.

In any case, I have no objection with this.

[marja, 2016/09/01 11:16:10]

Removed this one, calling impl()->ReportMessage directly. Not touching any other
ReportMessage funcs in this CL.

+
   void GetUnexpectedTokenMessage(
       Token::Value token, MessageTemplate::Template* message,
       Scanner::Location* location, const char** arg,
@@ -1166,6 +1173,11 @@ class ParserBase {
                               bool has_rest, int formals_start_pos,
                               int formals_end_pos, bool* ok);
 
+  BlockT ParseVariableDeclarations(VariableDeclarationContext var_context,
+                                   DeclarationParsingResult* parsing_result,
+                                   ZoneList<const AstRawString*>* names,
+                                   bool* ok);
+
   bool IsNextLetKeyword();
   bool IsTrivialExpression();
 
@@ -3399,6 +3411,153 @@ void ParserBase<Impl>::ParseFormalParameterList(FormalParametersT* parameters,
 }
 
 template <typename Impl>
+typename ParserBase<Impl>::BlockT ParserBase<Impl>::ParseVariableDeclarations(
+    VariableDeclarationContext var_context,
+    DeclarationParsingResult* parsing_result,
+    ZoneList<const AstRawString*>* names, bool* ok) {
+  // VariableDeclarations ::
+  //   ('var' | 'const' | 'let') (Identifier ('=' AssignmentExpression)?)+[',']
+  //
+  // ES6:
+  // FIXME(marja, nikolaos): Add an up-to-date comment about ES6 variable
+  // declaration syntax.
+
+  DeclarationParsingResult temp_result;
+  if (parsing_result == nullptr) {
+    parsing_result = &temp_result;
+  }
+  parsing_result->descriptor.declaration_kind = DeclarationDescriptor::NORMAL;
+  parsing_result->descriptor.declaration_pos = peek_position();
+  parsing_result->descriptor.initialization_pos = peek_position();
+  parsing_result->descriptor.mode = VAR;

[nickie, 2016/09/01 10:53:08]

Is there a reason why this line is not after 3441, inside the "if"?

[marja, 2016/09/01 11:16:10]

Done.

+
+  BlockT init_block = impl()->NullBlock();
+  if (var_context != kForStatement) {
+    init_block = impl()->NewBlock(nullptr, 1, true,
+                                  parsing_result->descriptor.declaration_pos);
+  }
+
+  if (peek() == Token::VAR) {

[nickie, 2016/09/01 10:53:08]

Again a matter of taste, but I'd rather use a "switch (peek())" here.  If you
prefer to keep the "if", I suggest that you turn the conditions to, e.g., "if
(Check(Token::VAR))" and remove the "Consume" calls.

[marja, 2016/09/01 11:16:10]

Done. (Replaced w/ switch)

+    Consume(Token::VAR);
+  } else if (peek() == Token::CONST) {
+    Consume(Token::CONST);
+    DCHECK(var_context != kStatement);
+    parsing_result->descriptor.mode = CONST;
+  } else if (peek() == Token::LET) {
+    Consume(Token::LET);
+    DCHECK(var_context != kStatement);
+    parsing_result->descriptor.mode = LET;
+  } else {
+    UNREACHABLE();  // by current callers
+  }
+
+  parsing_result->descriptor.scope = scope();
+  parsing_result->descriptor.hoist_scope = nullptr;
+
+  // The scope of a var/const declared variable anywhere inside a function
+  // is the entire function (ECMA-262, 3rd, 10.1.3, and 12.2). The scope
+  // of a let declared variable is the scope of the immediately enclosing
+  // block.
+  bool first_declaration = true;
+  int bindings_start = peek_position();
+  do {
+    // Parse binding pattern.
+    FuncNameInferrer::State fni_state(fni_);
+
+    if (!first_declaration) Consume(Token::COMMA);
+    first_declaration = false;

[nickie, 2016/09/01 10:53:08]

I think you can get rid of these two lines and the variable "first_declaration"
entirely, if you change the condition of the "do-while" in line 3551 to "while
(Check(Token::COMMA))".

[marja, 2016/09/01 11:16:10]

Done.

+
+    ExpressionT pattern = impl()->EmptyExpression();
+    int decl_pos = peek_position();
+    {
+      ExpressionClassifier pattern_classifier(this);
+      pattern = ParsePrimaryExpression(CHECK_OK_CUSTOM(NullBlock));
+
+      ValidateBindingPattern(CHECK_OK_CUSTOM(NullBlock));
+      if (IsLexicalVariableMode(parsing_result->descriptor.mode)) {
+        ValidateLetPattern(CHECK_OK_CUSTOM(NullBlock));
+      }
+    }
+
+    Scanner::Location variable_loc = scanner()->location();
+    bool single_name = impl()->IsIdentifier(pattern);
+
+    if (single_name && fni_ != nullptr) {
+      impl()->PushVariableName(fni_, impl()->AsIdentifier(pattern));

[nickie, 2016/09/01 10:53:08]

I think that impl()->PushVariableName does not need the first parameter (am I
right it will always be "fni_"?) and it could also incorporate the the "fni_ !=
nullptr" check.

[marja, 2016/09/01 11:16:10]

Leaving this as is, to keep PushLiteralName (already existing) and
PushVariableName uniform. I'll do a follow-up CL to get rid of fni in both.

+    }
+
+    ExpressionT value = impl()->EmptyExpression();
+    int initializer_position = kNoSourcePosition;
+    if (Check(Token::ASSIGN)) {
+      ExpressionClassifier classifier(this);
+      value = ParseAssignmentExpression(var_context != kForStatement,
+                                        CHECK_OK_CUSTOM(NullBlock));
+      impl()->RewriteNonPattern(CHECK_OK_CUSTOM(NullBlock));
+      variable_loc.end_pos = scanner()->location().end_pos;
+
+      if (!parsing_result->first_initializer_loc.IsValid()) {
+        parsing_result->first_initializer_loc = variable_loc;
+      }
+
+      // Don't infer if it is "a = function(){...}();"-like expression.
+      if (single_name && fni_ != nullptr) {
+        if (!value->IsCall() && !value->IsCallNew()) {
+          fni_->Infer();
+        } else {
+          fni_->RemoveLastFunction();
+        }
+      }
+
+      impl()->SetFunctionNameFromIdentifierRef(value, pattern);
+
+      // End position of the initializer is after the assignment expression.
+      initializer_position = scanner()->location().end_pos;
+    } else {
+      if (var_context != kForStatement || !PeekInOrOf()) {
+        // ES6 'const' and binding patterns require initializers.
+        if (parsing_result->descriptor.mode == CONST ||
+            !impl()->IsIdentifier(pattern)) {
+          ReportMessageAt(
+              Scanner::Location(decl_pos, scanner()->location().end_pos),
+              MessageTemplate::kDeclarationMissingInitializer,
+              !impl()->IsIdentifier(pattern) ? "destructuring" : "const");
+          *ok = false;
+          return impl()->NullBlock();
+        }
+        // 'let x' initializes 'x' to undefined.
+        if (parsing_result->descriptor.mode == LET) {
+          value = impl()->GetLiteralUndefined(position());
+        }
+      }
+
+      // End position of the initializer is after the variable.
+      initializer_position = position();
+    }
+
+    typename DeclarationParsingResult::Declaration decl(
+        pattern, initializer_position, value);
+    if (var_context == kForStatement) {
+      // Save the declaration for further handling in ParseForStatement.
+      parsing_result->declarations.Add(decl);
+    } else {
+      // Immediately declare the variable otherwise. This avoids O(N^2)
+      // behavior (where N is the number of variables in a single
+      // declaration) in the PatternRewriter having to do with removing
+      // and adding VariableProxies to the Scope (see bug 4699).
+      impl()->DeclareAndInitializeVariables(init_block,
+                                            &parsing_result->descriptor, &decl,
+                                            names, CHECK_OK_CUSTOM(NullBlock));
+    }
+  } while (peek() == Token::COMMA);
+
+  parsing_result->bindings_loc =
+      Scanner::Location(bindings_start, scanner()->location().end_pos);
+
+  DCHECK(*ok);

[nickie, 2016/09/01 10:53:08]

This line strikes me as a bit odd.  Is there any reason why we check this here
in particular?  It's definitely an invariant of all the parsing methods that,
when they return something meaningful, it is *ok == true.  Maybe we should
enforce it everywhere in the parser, e.g. by having a macro like
"RETURN(init_block);" that does this automatically.

Again, this is food for thought, not for this CL.

[marja, 2016/09/01 11:16:10]

I think this was just paranoid coding, as we use CHECK_OK above in all places
where *ok could be set to false. Leaving this as is.

+  return init_block;
+}
+
+template <typename Impl>
 void ParserBase<Impl>::CheckArityRestrictions(int param_count,
                                               FunctionKind function_kind,
                                               bool has_rest,