[parser] Refactor of Parse*Statement*, part 4

src/parsing/parser.h

Index: src/parsing/parser.h
diff --git a/src/parsing/parser.h b/src/parsing/parser.h
index 1ef3e61eebe1afa12bbf09a273234227a1e865f7..1a5e2f12312e659e1c7f8dc235456c33f174f5c9 100644
--- a/src/parsing/parser.h
+++ b/src/parsing/parser.h
@@ -157,6 +157,8 @@ struct ParserTypes<Parser> {
   typedef v8::internal::Statement* Statement;
   typedef ZoneList<v8::internal::Statement*>* StatementList;
   typedef v8::internal::Block* Block;
+  typedef v8::internal::BreakableStatement* BreakableStatementT;
+  typedef v8::internal::IterationStatement* IterationStatementT;
 
   // For constructing objects returned by the traversing functions.
   typedef AstNodeFactory Factory;
@@ -203,12 +205,6 @@ class Parser : public ParserBase<Parser> {
 
   enum class FunctionBodyType { kNormal, kSingleExpression };
 
-  DeclarationScope* GetDeclarationScope() const {
-    return scope()->GetDeclarationScope();
-  }
-  DeclarationScope* GetClosureScope() const {
-    return scope()->GetClosureScope();
-  }
   Variable* NewTemporary(const AstRawString* name) {
     return scope()->NewTemporary(name);
   }
@@ -285,6 +281,11 @@ class Parser : public ParserBase<Parser> {
       Block* block, const DeclarationDescriptor* declaration_descriptor,
       const DeclarationParsingResult::Declaration* declaration,
       ZoneList<const AstRawString*>* names, bool* ok);
+  ZoneList<const AstRawString*>* DeclareLabel(
+      ZoneList<const AstRawString*>* labels, Expression* expr, bool* ok);
+  bool ContainsLabel(ZoneList<const AstRawString*>* labels,
+                     const AstRawString* label);
+  Expression* RewriteReturn(Expression* return_value, int pos);
 
   DoExpression* ParseDoExpression(bool* ok);
   Expression* ParseYieldStarExpression(bool* ok);
@@ -370,17 +371,6 @@ class Parser : public ParserBase<Parser> {
     DEFINE_AST_VISITOR_MEMBERS_WITHOUT_STACKOVERFLOW()
   };
 
-  Statement* ParseExpressionOrLabelledStatement(
-      ZoneList<const AstRawString*>* labels,
-      AllowLabelledFunctionStatement allow_function, bool* ok);
-  IfStatement* ParseIfStatement(ZoneList<const AstRawString*>* labels,
-                                bool* ok);
-  Statement* ParseContinueStatement(bool* ok);
-  Statement* ParseBreakStatement(ZoneList<const AstRawString*>* labels,
-                                 bool* ok);
-  Statement* ParseReturnStatement(bool* ok);
-  Statement* ParseWithStatement(ZoneList<const AstRawString*>* labels,
-                                bool* ok);
   CaseClause* ParseCaseClause(bool* default_seen_ptr, bool* ok);
   Statement* ParseSwitchStatement(ZoneList<const AstRawString*>* labels,
                                   bool* ok);
@@ -641,9 +631,13 @@ class Parser : public ParserBase<Parser> {
            property->obj()->AsVariableProxy()->is_this();
   }
 
+  // This returns true if the expression is an indentifier,
+  // excluding the case of 'this' which has been converted to
+  // a variable proxy.
   V8_INLINE static bool IsIdentifier(Expression* expression) {
+    DCHECK_NOT_NULL(expression);
     VariableProxy* operand = expression->AsVariableProxy();
-    return operand != NULL && !operand->is_this();
+    return operand != nullptr && !operand->is_this();

[adamk, 2016/09/08 18:33:19]

Can you add a TODO to remove this null check now that we have the DCHECK? I
agree that we'd rather not change behavior in this patch, but it seems odd.

[nickie, 2016/09/09 09:42:27]

The DCHECK is for a non-existent AST node.  I think I introduced it here and
removed a check from some call sites.  It's definitely good here.

The check for "operand != nullptr" will fail if the expression is not a variable
proxy.  In this case, the function should return false.  We want to keep this.

[adamk, 2016/09/09 17:14:47]

Ah, indeed, I forgot how AsWhatever works, this is fine.

   }
 
   V8_INLINE static const AstRawString* AsIdentifier(Expression* expression) {
@@ -670,6 +664,13 @@ class Parser : public ParserBase<Parser> {
     return ObjectLiteral::IsBoilerplateProperty(property);
   }
 
+  V8_INLINE bool IsNative(Expression* expr) const {
+    return expr != nullptr && expr->AsVariableProxy() != nullptr &&

[adamk, 2016/09/08 18:33:19]

Can expr actually be NULL here?

[marja, 2016/09/09 08:44:59]

Style nit:

How about:

expr->IsVariableProxy() && expr->AsVariableProxy()-> ...

[nickie, 2016/09/09 09:42:26]

Done.  I also removed the !scanner()->literal_contains_escapes() and put it back
to the call site, where it belongs.  This method should not have unnecessary
side effects.

[nickie, 2016/09/09 09:42:27]

The DCHECK_NOT_NULL that I just introduced says no...  :-)
But we may not want to change the behavior.  Should I just add a TODO?

[adamk, 2016/09/09 17:14:47]

I'm fine without a TODO, this has very few callers so I think it's pretty safe
to change.

+           expr->AsVariableProxy()->raw_name() ==
+               ast_value_factory()->native_string() &&
+           !scanner()->literal_contains_escapes();
+  }
+
   V8_INLINE static bool IsArrayIndex(const AstRawString* string,
                                      uint32_t* index) {
     return string->AsArrayIndex(index);
@@ -823,6 +824,9 @@ class Parser : public ParserBase<Parser> {
 
   // "null" return type creators.
   V8_INLINE static const AstRawString* EmptyIdentifier() { return nullptr; }
+  V8_INLINE static bool IsEmptyIdentifier(const AstRawString* name) {
+    return name == nullptr;
+  }
   V8_INLINE static Expression* EmptyExpression() { return nullptr; }
   V8_INLINE static Literal* EmptyLiteral() { return nullptr; }
   V8_INLINE static ObjectLiteralProperty* EmptyObjectLiteralProperty() {
@@ -850,6 +854,7 @@ class Parser : public ParserBase<Parser> {
     return stmts == nullptr;
   }
   V8_INLINE static Statement* NullStatement() { return nullptr; }
+  V8_INLINE bool IsNullStatement(Statement* stmt) { return stmt == nullptr; }
   V8_INLINE bool IsNullOrEmptyStatement(Statement* stmt) {
     return stmt == nullptr || stmt->IsEmpty();
   }