Fix assertion in the parser about operator precedence.

runtime/vm/parser.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/parser.h"
 
 #include "lib/invocation_mirror.h"
 #include "vm/bigint_operations.h"
 #include "vm/bootstrap.h"
 #include "vm/class_finalizer.h"
@@ skipping 1869 matching lines @@
     ConsumeToken();
     AstNode* index_expr = ParseExpr(kAllowConst, kConsumeCascades);
     ExpectToken(Token::kRBRACK);
     AstNode* receiver = LoadReceiver(operator_pos);
     const Class& super_class = Class::ZoneHandle(current_class().SuperClass());
     ASSERT(!super_class.IsNull());
     super_op =
         new LoadIndexedNode(operator_pos, receiver, index_expr, super_class);
   } else {
     ASSERT(Token::CanBeOverloaded(CurrentToken()) ||
            (CurrentToken() == Token::kNE));

[Florian Schneider, 2013/09/18 15:09:09]

Could the ASSERT below even be removed since this assertion already asserts
valid super-operators?

[hausner, 2013/09/18 15:52:37]

If somebody decides tomorrow that the conditional operator should be
overloadable then this code breaks. That would be caught by the assertion below.

     Token::Kind op = CurrentToken();
     ConsumeToken();
 
     bool negate_result = false;
     if (op == Token::kNE) {
       op = Token::kEQ;
       negate_result = true;
     }
 
-    ASSERT(Token::Precedence(op) >= Token::Precedence(Token::kBIT_OR));
+    ASSERT(Token::Precedence(op) >= Token::Precedence(Token::kEQ));

[Kevin Millikin (Google), 2013/09/18 15:13:21]

This is OK to fix the tests.

It's kind of annoying to assert anything about precedence or to rely
specifically on a particular operator's precedence.

I probably find that annoying because overriding operators has basically nothing
at all to do with precedence.

[hausner, 2013/09/18 15:52:37]

We are blindly calling ParseBinaryExpr() here, so we better make sure that the
operator can be handled by that function. That's what this assert is about.
Maybe you find it a bit less annoying now.


On 2013/09/18 15:13:21, kmillikin wrote:

     AstNode* other_operand = ParseBinaryExpr(Token::Precedence(op) + 1);
 
     ArgumentListNode* op_arguments = new ArgumentListNode(operator_pos);
     AstNode* receiver = LoadReceiver(operator_pos);
     op_arguments->Add(receiver);
     op_arguments->Add(other_operand);
 
     // Resolve the operator function in the superclass.
     const String& operator_function_name =
         String::ZoneHandle(Symbols::New(Token::Str(op)));
@@ skipping 5427 matching lines @@
     arguments->Add(new LiteralNode(call_pos, array));
   }
   return MakeStaticCall(Symbols::NoSuchMethodError(),
                         PrivateCoreLibName(Symbols::ThrowNew()),
                         arguments);
 }
 
 
 AstNode* Parser::ParseBinaryExpr(int min_preced) {
   TRACE_PARSER("ParseBinaryExpr");
-  ASSERT(min_preced >= 4);
+  ASSERT(min_preced >= Token::Precedence(Token::kOR));
   AstNode* left_operand = ParseUnaryExpr();
   if (left_operand->IsPrimaryNode() &&
       (left_operand->AsPrimaryNode()->IsSuper())) {
     ErrorMsg(left_operand->token_pos(), "illegal use of 'super'");
   }
   int current_preced = Token::Precedence(CurrentToken());
   while (current_preced >= min_preced) {
     while (Token::Precedence(CurrentToken()) == current_preced) {
       Token::Kind op_kind = CurrentToken();
       const intptr_t op_pos = TokenPos();
@@ skipping 3066 matching lines @@
 void Parser::SkipQualIdent() {
   ASSERT(IsIdentifier());
   ConsumeToken();
   if (CurrentToken() == Token::kPERIOD) {
     ConsumeToken();  // Consume the kPERIOD token.
     ExpectIdentifier("identifier expected after '.'");
   }
 }
 
 }  // namespace dart