Move ParseVariableDeclarations to ParserBase.

src/parsing/parser-base.h

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef V8_PARSING_PARSER_BASE_H
 #define V8_PARSING_PARSER_BASE_H
 
 #include "src/ast/scopes.h"
 #include "src/bailout-reason.h"
 #include "src/base/hashmap.h"
@@ skipping 196 matching lines @@
   typedef typename Types::Expression ExpressionT;
   typedef typename Types::Identifier IdentifierT;
   typedef typename Types::FormalParameter FormalParameterT;
   typedef typename Types::FormalParameters FormalParametersT;
   typedef typename Types::FunctionLiteral FunctionLiteralT;
   typedef typename Types::Literal LiteralT;
   typedef typename Types::ObjectLiteralProperty ObjectLiteralPropertyT;
   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); }
   const Impl* impl() const { return static_cast<const Impl*>(this); }
 
   ParserBase(Zone* zone, Scanner* scanner, uintptr_t stack_limit,
              v8::Extension* extension, AstValueFactory* ast_value_factory,
              ParserRecorder* log)
       : scope_state_(nullptr),
         function_state_(nullptr),
@@ skipping 700 matching lines @@
     impl()->ReportMessageAt(source_location, message, arg, error_type);
   }
 
   void ReportMessageAt(Scanner::Location location,
                        MessageTemplate::Template message,
                        ParseErrorType error_type) {
     impl()->ReportMessageAt(location, message,
                             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,
       MessageTemplate::Template default_ = MessageTemplate::kUnexpectedToken);
 
   void ReportUnexpectedToken(Token::Value token);
   void ReportUnexpectedTokenAt(
       Scanner::Location location, Token::Value token,
       MessageTemplate::Template message = MessageTemplate::kUnexpectedToken);
 
@@ skipping 212 matching lines @@
   ExpressionT ParseTemplateLiteral(ExpressionT tag, int start, bool* ok);
   ExpressionT ParseSuperExpression(bool is_new, bool* ok);
   ExpressionT ParseNewTargetExpression(bool* ok);
 
   void ParseFormalParameter(FormalParametersT* parameters, bool* ok);
   void ParseFormalParameterList(FormalParametersT* parameters, bool* ok);
   void CheckArityRestrictions(int param_count, FunctionKind function_type,
                               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();
 
   // Checks if the expression is a valid reference expression (e.g., on the
   // left-hand side of assignments). Although ruled out by ECMA as early errors,
   // we allow calls for web compatibility and rewrite them to a runtime throw.
   ExpressionT CheckAndRewriteReferenceExpression(
       ExpressionT expression, int beg_pos, int end_pos,
       MessageTemplate::Template message, bool* ok);
   ExpressionT CheckAndRewriteReferenceExpression(
@@ skipping 2213 matching lines @@
     }
   }
 
   for (int i = 0; i < parameters->Arity(); ++i) {
     auto parameter = parameters->at(i);
     impl()->DeclareFormalParameter(parameters->scope, parameter);
   }
 }
 
 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,
                                               int formals_start_pos,
                                               int formals_end_pos, bool* ok) {
   if (IsGetterFunction(function_kind)) {
     if (param_count != 0) {
       impl()->ReportMessageAt(
           Scanner::Location(formals_start_pos, formals_end_pos),
           MessageTemplate::kBadGetterArity);
@@ skipping 380 matching lines @@
     has_seen_constructor_ = true;
     return;
   }
 }
 
 
 }  // namespace internal
 }  // namespace v8
 
 #endif  // V8_PARSING_PARSER_BASE_H