[cleanup] refactor ParsePropertyDefinition for clarity

src/preparser.h

Index: src/preparser.h
diff --git a/src/preparser.h b/src/preparser.h
index 9f2b13212c6526f7d130502e0d08a315177671fb..f79a868848c8aea9e41f9ed0b0d35e0053eb1d0b 100644
--- a/src/preparser.h
+++ b/src/preparser.h
@@ -2611,19 +2611,62 @@ ParserBase<Traits>::ParsePropertyDefinition(
     this->PushLiteralName(fni_, name);
   }
 
-  if (!in_class && !is_generator && peek() == Token::COLON) {
-    // PropertyDefinition : PropertyName ':' AssignmentExpression
-    if (!*is_computed_name) {
-      checker->CheckProperty(name_token, kValueProperty, is_static,
-                             is_generator,
-                             CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+  if (!in_class && !is_generator) {
+    if (peek() == Token::COLON) {

[adamk, 2015/09/16 19:03:14]

Please add

DCHECK(!is_static)

above this line.

[caitp (gmail), 2015/09/16 20:10:25]

Done.

+      // PropertyDefinition
+      //     PropertyName : AssignmentExpression

[adamk, 2015/09/16 19:03:14]

Please quote the ':' in this production (otherwise it reads ambiguously)

[caitp (gmail), 2015/09/16 20:10:25]

Done.

+      if (!*is_computed_name) {
+        checker->CheckProperty(name_token, kValueProperty, false, false,
+                               CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+      }
+      Consume(Token::COLON);
+      value = this->ParseAssignmentExpression(
+          true, classifier, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+      return factory()->NewObjectLiteralProperty(name_expression, value, false,
+                                                 *is_computed_name);
+    } else if (Token::IsIdentifier(name_token, language_mode(),

[adamk, 2015/09/16 19:03:13]

Style nit: I think it would be easier to read this as a separate if statement,
given that you're returning from the 'if' branch.

[caitp (gmail), 2015/09/16 20:10:25]

Done.

+                                   this->is_generator()) &&
+               (peek() == Token::COMMA || peek() == Token::RBRACE ||
+                peek() == Token::ASSIGN)) {
+      // PropertyDefinition
+      //     IdentifierReference
+      //     CoverInitializedName
+      //
+      // CoverInitializedName
+      //     IdentifierReference : AssignmentExpression

[adamk, 2015/09/16 19:03:14]

Do you mean

"IdentifierReference Initializer"

?

[caitp (gmail), 2015/09/16 20:10:25]

Yes, not sure how this got so mixed up =) Done --- I dunno if you prefer `(opt)`
as in the draft, or a `?` as it usually shows up in yacc/bison, or what. I went
with (opt), but it looks weird without being in subscript.

[adamk, 2015/09/16 20:58:00]

I'd go with the ?, that's what we use elsewhere in this file.

[caitp (gmail), 2015/09/16 21:01:09]

Done.

+      if (classifier->duplicate_finder() != nullptr &&
+          scanner()->FindSymbol(classifier->duplicate_finder(), 1) != 0) {
+        classifier->RecordDuplicateFormalParameterError(scanner()->location());
+      }
+
+      ExpressionT lhs = this->ExpressionFromIdentifier(
+          name, next_beg_pos, next_end_pos, scope_, factory());
+
+      if (peek() == Token::ASSIGN) {
+        this->ExpressionUnexpectedToken(classifier);
+        Consume(Token::ASSIGN);
+        ExpressionClassifier rhs_classifier;
+        ExpressionT rhs = this->ParseAssignmentExpression(
+            true, &rhs_classifier, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+        classifier->Accumulate(rhs_classifier,
+                               ExpressionClassifier::ExpressionProductions);
+        value = factory()->NewAssignment(Token::ASSIGN, lhs, rhs,
+                                         RelocInfo::kNoPosition);
+      } else {
+        value = lhs;
+      }
+
+      return factory()->NewObjectLiteralProperty(
+          name_expression, value, ObjectLiteralProperty::COMPUTED, false,
+          false);
     }
-    Consume(Token::COLON);
-    value = this->ParseAssignmentExpression(
-        true, classifier, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
+  }
+
 
-  } else if (is_generator || peek() == Token::LPAREN) {
-    // Concise Method
+  if (is_generator || peek() == Token::LPAREN) {
+    // MethodDefinition
+    //     PropertyName ( StrictFormalParameters ) { FunctionBody }

[adamk, 2015/09/16 19:03:14]

More quoting, please. Not going to mark up the rest of these, but all literal
tokens should be quoted.

[caitp (gmail), 2015/09/16 20:10:25]

Done.

+    //     * PropertyName ( StrictFormalParameters ) { FunctionBody }
     if (!*is_computed_name) {
       checker->CheckProperty(name_token, kMethodProperty, is_static,
                              is_generator,
@@ -2650,15 +2693,15 @@ ParserBase<Traits>::ParsePropertyDefinition(
     return factory()->NewObjectLiteralProperty(name_expression, value,
                                                ObjectLiteralProperty::COMPUTED,
                                                is_static, *is_computed_name);
-
   } else if (in_class && name_is_static && !is_static) {

[adamk, 2015/09/16 19:03:14]

Nit again: I'd recommend making this not an 'else' clause but a separate if
statement, given that the return above.

[caitp (gmail), 2015/09/16 20:10:25]

Done.

-    // static MethodDefinition
+    // ClassElement (static)
+    //     static MethodDefinition
     return ParsePropertyDefinition(checker, true, has_extends, true,
                                    is_computed_name, nullptr, classifier, ok);
-  } else if ((is_get || is_set) &&
-             (in_class || (peek() != Token::RBRACE && peek() != Token::COMMA &&
-                           peek() != Token::ASSIGN))) {
-    // Accessor
+  } else if (is_get || is_set) {
+    // MethodDefinition (Accessors)
+    //     get PropertyName ( ) { FunctionBody }
+    //     set PropertyName ( PropertySetParameterList ) { FunctionBody }
     name = this->EmptyIdentifier();
     bool dont_care = false;
     name_token = peek();
@@ -2693,35 +2736,6 @@ ParserBase<Traits>::ParsePropertyDefinition(
         name_expression, value,
         is_get ? ObjectLiteralProperty::GETTER : ObjectLiteralProperty::SETTER,
         is_static, *is_computed_name);
-
-  } else if (!in_class && Token::IsIdentifier(name_token, language_mode(),
-                                              this->is_generator())) {
-    DCHECK(!*is_computed_name);
-    DCHECK(!is_static);
-
-    if (classifier->duplicate_finder() != nullptr &&
-        scanner()->FindSymbol(classifier->duplicate_finder(), 1) != 0) {
-      classifier->RecordDuplicateFormalParameterError(scanner()->location());
-    }
-
-    ExpressionT lhs = this->ExpressionFromIdentifier(
-        name, next_beg_pos, next_end_pos, scope_, factory());
-    if (peek() == Token::ASSIGN) {
-      this->ExpressionUnexpectedToken(classifier);
-      Consume(Token::ASSIGN);
-      ExpressionClassifier rhs_classifier;
-      ExpressionT rhs = this->ParseAssignmentExpression(
-          true, &rhs_classifier, CHECK_OK_CUSTOM(EmptyObjectLiteralProperty));
-      classifier->Accumulate(rhs_classifier,
-                             ExpressionClassifier::ExpressionProductions);
-      value = factory()->NewAssignment(Token::ASSIGN, lhs, rhs,
-                                       RelocInfo::kNoPosition);
-    } else {
-      value = lhs;
-    }
-    return factory()->NewObjectLiteralProperty(
-        name_expression, value, ObjectLiteralProperty::COMPUTED, false, false);
-
   } else {

[adamk, 2015/09/16 19:03:13]

You can move the contents of the else clause up to the function level...

[caitp (gmail), 2015/09/16 20:10:25]

Done.

     Token::Value next = Next();
     ReportUnexpectedToken(next);
@@ -2729,8 +2743,7 @@ ParserBase<Traits>::ParsePropertyDefinition(
     return this->EmptyObjectLiteralProperty();
   }
 
-  return factory()->NewObjectLiteralProperty(name_expression, value, is_static,
-                                             *is_computed_name);
+  UNREACHABLE();

[adamk, 2015/09/16 19:03:13]

...and remove this line.

[caitp (gmail), 2015/09/16 20:10:25]

Done.

 }