[clang] SequenceChecker: C++17 sequencing rule for overloaded operators.

[riccibruno]

Summary

In C++17 the operand(s) of an overloaded operator are sequenced as for the corresponding built-in operator when the overloaded operator is called with the operator notation ([over.match.oper]p2).

Reported in PR35340.

[riccibruno]

@rsmith Friendly ping on this patch and D81003.

[rsmith]

It would be nice to avoid the duplication between the builtin and overloaded cases here, but I don't see a good way to do that. LGTM.

clang/lib/Sema/SemaChecking.cpp

SemaRef.runWithSufficientStackSpace(CE->getExprLoc(), [&] {
       Region = OldRegion;
       if (SemaRef.getLangOpts().CPlusPlus17) {
         Tree.merge(CalleeRegion);
         Tree.merge(OtherRegion);
       }
     });
   }
 
+  void VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *CXXOCE) {
+    // C++17 [over.match.oper]p2:
+    //   [...] the operator notation is first transformed to the equivalent
+    //   function-call notation as summarized in Table 12 (where @ denotes one
+    //   of the operators covered in the specified subclause). However, the
+    //   operands are sequenced in the order prescribed for the built-in
+    //   operator (Clause 8).
+    //
+    // From the above only overloaded binary operators and overloaded call
+    // operators have sequencing rules in C++17 that we need to handle
+    // separately.
+    if (!SemaRef.getLangOpts().CPlusPlus17 ||
+        (CXXOCE->getNumArgs() != 2 && CXXOCE->getOperator() != OO_Call))
+      return VisitCallExpr(CXXOCE);
+
+    enum {
+      NoSequencing,
+      LHSBeforeRHS,
+      RHSBeforeLHS,
+      LHSBeforeRest
+    } SequencingKind;
+    switch (CXXOCE->getOperator()) {
+    case OO_Equal:
+    case OO_PlusEqual:
+    case OO_MinusEqual:
+    case OO_StarEqual:
+    case OO_SlashEqual:
+    case OO_PercentEqual:
+    case OO_CaretEqual:
+    case OO_AmpEqual:
+    case OO_PipeEqual:
+    case OO_LessLessEqual:
+    case OO_GreaterGreaterEqual:
+      SequencingKind = RHSBeforeLHS;
+      break;
+
+    case OO_LessLess:
+    case OO_GreaterGreater:
+    case OO_AmpAmp:
+    case OO_PipePipe:
+    case OO_Comma:
+    case OO_ArrowStar:
+    case OO_Subscript:
+      SequencingKind = LHSBeforeRHS;
+      break;
+
+    case OO_Call:
+      SequencingKind = LHSBeforeRest;
+      break;
+
+    default:
+      SequencingKind = NoSequencing;
+      break;
+    }
+
+    if (SequencingKind == NoSequencing)
+      return VisitCallExpr(CXXOCE);
+
+    // This is a call, so all subexpressions are sequenced before the result.
+    SequencedSubexpression Sequenced(*this);
+
+    SemaRef.runWithSufficientStackSpace(CXXOCE->getExprLoc(), [&] {
+      assert(SemaRef.getLangOpts().CPlusPlus17 &&
+             "Should only get there with C++17 and above!");
+      assert((CXXOCE->getNumArgs() == 2 || CXXOCE->getOperator() == OO_Call) &&
+             "Should only get there with an overloaded binary operator"
+             " or an overloaded call operator!");
+
+      if (SequencingKind == LHSBeforeRest) {
+        assert(CXXOCE->getOperator() == OO_Call &&
+               "We should only have an overloaded call operator here!");
+
+        // This is very similar to VisitCallExpr, except that we only have the
+        // C++17 case. The postfix-expression is the first argument of the
+        // CXXOperatorCallExpr. The expressions in the expression-list, if any,
+        // are in the following arguments.
+        //
+        // Note that we intentionally do not visit the callee expression since
+        // it is just a decayed reference to a function.
+        SequenceTree::Seq PostfixExprRegion = Tree.allocate(Region);
+        SequenceTree::Seq ArgsRegion = Tree.allocate(Region);
+        SequenceTree::Seq OldRegion = Region;
+
+        assert(CXXOCE->getNumArgs() >= 1 &&
+               "An overloaded call operator must have at least one argument"
+               " for the postfix-expression!");
+        const Expr *PostfixExpr = CXXOCE->getArgs()[0];
+        llvm::ArrayRef<const Expr *> Args(CXXOCE->getArgs() + 1,
+                                          CXXOCE->getNumArgs() - 1);
+
+        // Visit the postfix-expression first.
+        {
+          Region = PostfixExprRegion;
+          SequencedSubexpression Sequenced(*this);
+          Visit(PostfixExpr);
+        }
+
+        // Then visit the argument expressions.
+        Region = ArgsRegion;
+        for (const Expr *Arg : Args)
+          Visit(Arg);
+
+        Region = OldRegion;
+        Tree.merge(PostfixExprRegion);
+        Tree.merge(ArgsRegion);
+      } else {
+        assert(CXXOCE->getNumArgs() == 2 &&
+               "Should only have two arguments here!");
+        assert((SequencingKind == LHSBeforeRHS ||
+                SequencingKind == RHSBeforeLHS) &&
+               "Unexpected sequencing kind!");
+
+        // We do not visit the callee expression since it is just a decayed
+        // reference to a function.
+        const Expr *E1 = CXXOCE->getArg(0);
+        const Expr *E2 = CXXOCE->getArg(1);
+        if (SequencingKind == RHSBeforeLHS)
+          std::swap(E1, E2);
+
+        return VisitSequencedExpressions(E1, E2);
+      }
+    });
+  }
+
   void VisitCXXConstructExpr(const CXXConstructExpr *CCE) {
     // This is a call, so all subexpressions are sequenced before the result.
     SequencedSubexpression Sequenced(*this);
 
     if (!CCE->isListInitialization())
       return VisitExpr(CCE);
 
     // In C++11, list initializations are sequenced.

clang/test/SemaCXX/warn-unsequenced.cpp

namespace PR20819 {
   foo get_foo(int);
 
   void g() {
     int a = 0;
     get_foo(a).bar(a++);  // cxx11-warning {{unsequenced modification and access to 'a'}}
   }
 }
 
+namespace overloaded_operators {
+  struct E {
+    E &operator=(E &);
+    E operator()(E);
+    E operator()(E, E);
+    E operator[](E);
+  } e;
+  // Binary operators with unsequenced operands.
+  E operator+(E,E);
+  E operator-(E,E);
+  E operator*(E,E);
+  E operator/(E,E);
+  E operator%(E,E);
+  E operator^(E,E);
+  E operator&(E,E);
+  E operator|(E,E);
+
+  E operator<(E,E);
+  E operator>(E,E);
+  E operator==(E,E);
+  E operator!=(E,E);
+  E operator>=(E,E);
+  E operator<=(E,E);
+
+  // Binary operators where the RHS is sequenced before the LHS in C++17.
+  E operator+=(E,E);
+  E operator-=(E,E);
+  E operator*=(E,E);
+  E operator/=(E,E);
+  E operator%=(E,E);
+  E operator^=(E,E);
+  E operator&=(E,E);
+  E operator|=(E,E);
+  E operator<<=(E,E);
+  E operator>>=(E,E);
+
+  // Binary operators where the LHS is sequenced before the RHS in C++17.
+  E operator<<(E,E);
+  E operator>>(E,E);
+  E operator&&(E,E);
+  E operator||(E,E);
+  E operator,(E,E);
+  E operator->*(E,E);
+
+  void test() {
+    int i = 0;
+    // Binary operators with unsequenced operands.
+    ((void)i++,e) + ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) - ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) * ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) / ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) % ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) ^ ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) & ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) | ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+
+    ((void)i++,e) < ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) > ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) == ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) != ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) <= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) >= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+
+    // Binary operators where the RHS is sequenced before the LHS in C++17.
+    ((void)i++,e) = ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) += ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) -= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) *= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) /= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) %= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) ^= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) &= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) |= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) <<= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) >>= ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+
+    operator+=(((void)i++,e), ((void)i++,e));
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+
+    // Binary operators where the LHS is sequenced before the RHS in C++17.
+    ((void)i++,e) << ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) >> ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) || ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) && ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e) , ((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    ((void)i++,e)->*((void)i++,e);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+
+    operator<<(((void)i++,e), ((void)i++,e));
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+
+    ((void)i++,e)[((void)i++,e)];
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+
+    ((void)i++,e)(((void)i++,e));
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    e(((void)i++,e), ((void)i++,e));
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+
+    ((void)i++,e).operator()(((void)i++,e));
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+
+  }
+}
+
+namespace PR35340 {
+  struct S {};
+  S &operator<<(S &, int);
+
+  void test() {
+    S s;
+    int i = 0;
+    s << i++ << i++;
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+
+    operator<<(operator<<(s, i++), i++);
+    // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+    // cxx17-warning@-2 {{multiple unsequenced modifications to 'i'}}
+  }
+}
+
 namespace members {
 
 struct S1 {
   unsigned bf1 : 2;
   unsigned bf2 : 2;
   unsigned a;
   unsigned b;
   static unsigned x;
int Foo<X>::Run() {
   // the builtin operator evaluatation is well-ordered.  Waiting until
   // instantiation to check these expressions will prevent false positives.
   if ((num = bar.get()) < 5 && num < 10) { }
   if ((num = bar.get()) < 5 || num < 10) { }
   if (static_cast<E>((num = bar.get()) < 5) || static_cast<E>(num < 10)) { }
 
   if (static_cast<E>((num = bar.get()) < 5) && static_cast<E>(num < 10)) { }
   // cxx11-warning@-1 {{unsequenced modification and access to 'num'}}
-  // cxx17-warning@-2 {{unsequenced modification and access to 'num'}}
 
   foo(num++, num++);
   // cxx11-warning@-1 {{multiple unsequenced modifications to 'num'}}
   // cxx17-warning@-2 {{multiple unsequenced modifications to 'num'}}
   return 1;
 }
 
 int x = Foo<int>().Run();
 // cxx11-note@-1 {{in instantiation of member function 'templates::Foo<int>::Run'}}
 // cxx17-note@-2 {{in instantiation of member function 'templates::Foo<int>::Run'}}
 
 
 template <typename T>
 int Run2() {
   T t = static_cast<T>(0);
   return (t = static_cast<T>(1)) && t;
   // cxx11-warning@-1 {{unsequenced modification and access to 't'}}
-  // cxx17-warning@-2 {{unsequenced modification and access to 't'}}
 }
 
 int y = Run2<bool>();
 int z = Run2<E>();
 // cxx11-note@-1{{in instantiation of function template specialization 'templates::Run2<templates::E>' requested here}}
-// cxx17-note@-2{{in instantiation of function template specialization 'templates::Run2<templates::E>' requested here}}
 
 template <typename T> int var = sizeof(T);
 void test_var() {
   var<int>++ + var<int>++; // cxx11-warning {{multiple unsequenced modifications to 'var<int>'}}
                            // cxx17-warning@-1 {{multiple unsequenced modifications to 'var<int>'}}
   var<int>++ + var<int>; // cxx11-warning {{unsequenced modification and access to 'var<int>'}}
                          // cxx17-warning@-1 {{unsequenced modification and access to 'var<int>'}}
   int &r = var<int>;
   r++ + var<int>++; // no-warning TODO {{multiple unsequenced modifications to 'var<int>'}}
   r++ + var<long>++; // no-warning
 }
 
 } // namespace templates