[Sema] SequenceChecker: C++17 sequencing rules for built-in operators <<, >>, .*, ->*, =, op=

Implement the C++17 sequencing rules for the built-in operators <<, >>, .*,
 ->*, = and op=.

Differential Revision: https://reviews.llvm.org/D58297

Reviewed By: rsmith

Author: riccibruno

clang/lib/Sema/SemaChecking.cpp

@@ -12832,8 +12832,37 @@ class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
     //   expression E1 is sequenced before the expression E2.
     if (SemaRef.getLangOpts().CPlusPlus17)
       VisitSequencedExpressions(ASE->getLHS(), ASE->getRHS());
-    else
-      Base::VisitStmt(ASE);
+    else {
+      Visit(ASE->getLHS());
+      Visit(ASE->getRHS());
+    }
+  }
+
+  void VisitBinPtrMemD(const BinaryOperator *BO) { VisitBinPtrMem(BO); }
+  void VisitBinPtrMemI(const BinaryOperator *BO) { VisitBinPtrMem(BO); }
+  void VisitBinPtrMem(const BinaryOperator *BO) {
+    // C++17 [expr.mptr.oper]p4:
+    //  Abbreviating pm-expression.*cast-expression as E1.*E2, [...]
+    //  the expression E1 is sequenced before the expression E2.
+    if (SemaRef.getLangOpts().CPlusPlus17)
+      VisitSequencedExpressions(BO->getLHS(), BO->getRHS());
+    else {
+      Visit(BO->getLHS());
+      Visit(BO->getRHS());
+    }
+  }
+
+  void VisitBinShl(const BinaryOperator *BO) { VisitBinShlShr(BO); }
+  void VisitBinShr(const BinaryOperator *BO) { VisitBinShlShr(BO); }
+  void VisitBinShlShr(const BinaryOperator *BO) {
+    // C++17 [expr.shift]p4:
+    //  The expression E1 is sequenced before the expression E2.
+    if (SemaRef.getLangOpts().CPlusPlus17)
+      VisitSequencedExpressions(BO->getLHS(), BO->getRHS());
+    else {
+      Visit(BO->getLHS());
+      Visit(BO->getRHS());
+    }
   }
 
   void VisitBinComma(const BinaryOperator *BO) {
@@ -12845,38 +12874,67 @@ class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
   }
 
   void VisitBinAssign(const BinaryOperator *BO) {
-    // The modification is sequenced after the value computation of the LHS
-    // and RHS, so check it before inspecting the operands and update the
+    SequenceTree::Seq RHSRegion;
+    SequenceTree::Seq LHSRegion;
+    if (SemaRef.getLangOpts().CPlusPlus17) {
+      RHSRegion = Tree.allocate(Region);
+      LHSRegion = Tree.allocate(Region);
+    } else {
+      RHSRegion = Region;
+      LHSRegion = Region;
+    }
+    SequenceTree::Seq OldRegion = Region;
+
+    // C++11 [expr.ass]p1:
+    //  [...] the assignment is sequenced after the value computation
+    //  of the right and left operands, [...]
+    //
+    // so check it before inspecting the operands and update the
     // map afterwards.
-    Object O = getObject(BO->getLHS(), true);
-    if (!O)
-      return VisitExpr(BO);
+    Object O = getObject(BO->getLHS(), /*Mod=*/true);
+    if (O)
+      notePreMod(O, BO);
+
+    if (SemaRef.getLangOpts().CPlusPlus17) {
+      // C++17 [expr.ass]p1:
+      //  [...] The right operand is sequenced before the left operand. [...]
+      {
+        SequencedSubexpression SeqBefore(*this);
+        Region = RHSRegion;
+        Visit(BO->getRHS());
+      }
 
-    notePreMod(O, BO);
+      Region = LHSRegion;
+      Visit(BO->getLHS());
 
-    // C++11 [expr.ass]p7:
-    //   E1 op= E2 is equivalent to E1 = E1 op E2, except that E1 is evaluated
-    //   only once.
-    //
-    // Therefore, for a compound assignment operator, O is considered used
-    // everywhere except within the evaluation of E1 itself.
-    if (isa<CompoundAssignOperator>(BO))
-      notePreUse(O, BO);
+      if (O && isa<CompoundAssignOperator>(BO))
+        notePostUse(O, BO);
 
-    Visit(BO->getLHS());
+    } else {
+      // C++11 does not specify any sequencing between the LHS and RHS.
+      Region = LHSRegion;
+      Visit(BO->getLHS());
 
-    if (isa<CompoundAssignOperator>(BO))
-      notePostUse(O, BO);
+      if (O && isa<CompoundAssignOperator>(BO))
+        notePostUse(O, BO);
 
-    Visit(BO->getRHS());
+      Region = RHSRegion;
+      Visit(BO->getRHS());
+    }
 
     // C++11 [expr.ass]p1:
-    //   the assignment is sequenced [...] before the value computation of the
-    //   assignment expression.
+    //  the assignment is sequenced [...] before the value computation of the
+    //  assignment expression.
     // C11 6.5.16/3 has no such rule.
-    notePostMod(O, BO,
-                SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
-                                                : UK_ModAsSideEffect);
+    Region = OldRegion;
+    if (O)
+      notePostMod(O, BO,
+                  SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
+                                                  : UK_ModAsSideEffect);
+    if (SemaRef.getLangOpts().CPlusPlus17) {
+      Tree.merge(RHSRegion);
+      Tree.merge(LHSRegion);
+    }
   }
 
   void VisitCompoundAssignOperator(const CompoundAssignOperator *CAO) {

clang/test/CXX/drs/dr2xx.cpp

@@ -225,11 +225,17 @@ namespace dr222 { // dr222: dup 637
     void((a += b) += c);
     void((a += b) + (a += c)); // expected-warning {{multiple unsequenced modifications to 'a'}}
 
-    x[a++] = a; // expected-warning {{unsequenced modification and access to 'a'}}
+    x[a++] = a;
+#if __cplusplus < 201703L
+    // expected-warning@-2 {{unsequenced modification and access to 'a'}}
+#endif
 
     a = b = 0; // ok, read and write of 'b' are sequenced
 
-    a = (b = a++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+    a = (b = a++);
+#if __cplusplus < 201703L
+    // expected-warning@-2 {{multiple unsequenced modifications to 'a'}}
+#endif
     a = (b = ++a);
 #pragma clang diagnostic pop
   }

clang/test/CXX/drs/dr6xx.cpp

@@ -317,7 +317,10 @@ namespace dr635 { // dr635: yes
 namespace dr637 { // dr637: yes
   void f(int i) {
     i = ++i + 1;
-    i = i++ + 1; // expected-warning {{unsequenced}}
+    i = i++ + 1;
+#if __cplusplus < 201703L
+    // expected-warning@-2 {{unsequenced}}
+#endif
   }
 }
 

clang/test/SemaCXX/warn-unsequenced.cpp

@@ -26,7 +26,6 @@ void test() {
   a + a++; // cxx11-warning {{unsequenced modification and access to 'a'}}
            // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   a = a++; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-           // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   ++ ++a; // ok
   (a++, a++); // ok
   ++a + ++a; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
@@ -36,13 +35,10 @@ void test() {
   (a++, a) = 0; // ok, increment is sequenced before value computation of LHS
   a = xs[++a]; // ok
   a = xs[a++]; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-               // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   (a ? xs[0] : xs[1]) = ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-                             // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   a = (++a, ++a); // ok
   a = (a++, ++a); // ok
   a = (a++, a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-                  // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   f(a, a); // ok
   f(a = 0, a); // cxx11-warning {{unsequenced modification and access to 'a'}}
                // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
@@ -61,7 +57,6 @@ void test() {
   (++a, a) += 1; // ok
   a = ++a; // ok
   a += ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-            // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
   A agg1 = { a++, a++ }; // ok
   A agg2 = { a++ + a, a++ }; // cxx11-warning {{unsequenced modification and access to 'a'}}
@@ -77,7 +72,6 @@ void test() {
   a = S { ++a, a++ }.n; // ok
   A { ++a, a++ }.x; // ok
   a = A { ++a, a++ }.x; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-                        // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   A { ++a, a++ }.x + A { ++a, a++ }.y; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
                                        // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
 
@@ -112,14 +106,10 @@ void test() {
   a += (a++, a) + a; // cxx11-warning {{unsequenced modification and access to 'a'}}
                      // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
-  int *p = xs;
-  a = *(a++, p); // ok
   a = a++ && a; // ok
-  p[(long long unsigned)(p = 0)]; // cxx11-warning {{unsequenced modification and access to 'p'}}
 
   A *q = &agg1;
   (q = &agg2)->y = q->x; // cxx11-warning {{unsequenced modification and access to 'q'}}
-                         // TODO cxx17-warning@-1 {{unsequenced modification and access to 'q'}}
 
   // This has undefined behavior if a == 0; otherwise, the side-effect of the
   // increment is sequenced before the value computation of 'f(a, a)', which is
@@ -147,20 +137,14 @@ void test() {
   xs[8] ? ++a : a++; // no-warning
   xs[8] ? a+=1 : a+= 2; // no-warning
   (xs[8] ? a+=1 : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-                              // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (xs[8] ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-                          // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (xs[8] ? a : a+= 2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-                           // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   a = (xs[8] ? a+=1 : a+= 2); // no-warning
   a += (xs[8] ? a+=1 : a+= 2); // cxx11-warning {{unsequenced modification and access to 'a'}}
-                               // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
 
   (false ? a+=1 : a) = a; // no-warning
   (true ? a+=1 : a) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-                         // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (false ? a : a+=2) = a; // cxx11-warning {{unsequenced modification and access to 'a'}}
-                          // TODO cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   (true ? a : a+=2) = a; // no-warning
 
   xs[8] && (++a + a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
@@ -180,19 +164,15 @@ void test() {
   a = ((a++, false) || (a++, false) || (a++, false) || (a++, false)); // no-warning
   a = ((a++, true) && (a++, true) && (a++, true) && (a++, true)); // no-warning
   a = ((a++, false) || (a++, false) || (a++, false) || a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-                                                             // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   a = ((a++, true) && (a++, true) && (a++, true) && a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-                                                          // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   a = ((a++, false) || (a++, false) || (a++, false) || (a + a, false)); // no-warning
   a = ((a++, true) && (a++, true) && (a++, true) && (a + a, true)); // no-warning
 
   a = (false && a++); // no-warning
   a = (true && a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-                     // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   a = (true && ++a); // no-warning
   a = (true || a++); // no-warning
   a = (false || a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
-                      // TODO cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
   a = (false || ++a); // no-warning
 
   (a++) | (a++); // cxx11-warning {{multiple unsequenced modifications to 'a'}}
@@ -207,6 +187,75 @@ void test() {
   (__builtin_object_size(&(++a, a), 0) ? 1 : 0) + ++a; // ok
   (__builtin_expect(++a, 0) ? 1 : 0) + ++a; // cxx11-warning {{multiple unsequenced modifications to 'a'}}
                                             // cxx17-warning@-1 {{multiple unsequenced modifications to 'a'}}
+
+
+  int *p = xs;
+  a = *(a++, p); // no-warning
+  p[(long long unsigned)(p = 0)]; // cxx11-warning {{unsequenced modification and access to 'p'}}
+  (i++, xs)[i++]; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  (++i, xs)[++i]; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  (i, xs)[++i + ++i]; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+                      // cxx17-warning@-1 {{multiple unsequenced modifications to 'i'}}
+  p++[p == xs]; // cxx11-warning {{unsequenced modification and access to 'p'}}
+  ++p[p++ == xs]; // cxx11-warning {{unsequenced modification and access to 'p'}}
+
+  struct S { int x; } s, *ps = &s;
+  int (S::*PtrMem);
+  (PtrMem = &S::x ,s).*(PtrMem); // cxx11-warning {{unsequenced modification and access to 'PtrMem'}}
+  (PtrMem = &S::x ,s).*(PtrMem = &S::x); // cxx11-warning {{multiple unsequenced modifications to 'PtrMem'}}
+  (PtrMem = &S::x ,ps)->*(PtrMem); // cxx11-warning {{unsequenced modification and access to 'PtrMem'}}
+  (PtrMem = &S::x ,ps)->*(PtrMem = &S::x); // cxx11-warning {{multiple unsequenced modifications to 'PtrMem'}}
+  (PtrMem = nullptr) == (PtrMem = nullptr); // cxx11-warning {{multiple unsequenced modifications to 'PtrMem'}}
+                                            // cxx17-warning@-1 {{multiple unsequenced modifications to 'PtrMem'}}
+  (PtrMem = nullptr) == PtrMem; // cxx11-warning {{unsequenced modification and access to 'PtrMem'}}
+                                // cxx17-warning@-1 {{unsequenced modification and access to 'PtrMem'}}
+
+  i++ << i++; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  ++i << ++i; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  i++ << i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i << i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i++ >> i++; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  ++i >> ++i; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  i++ >> i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i >> i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  (i++ << i) + i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+                  // cxx17-warning@-1 {{unsequenced modification and access to 'i'}}
+  (i++ << i) << i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+
+  ++i = i++; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  i = i+= 1; // no-warning
+  i = i++ + ++i; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+                 // cxx17-warning@-1 {{multiple unsequenced modifications to 'i'}}
+  ++i += ++i; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  ++i += i++; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  (i++, i) += ++i; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  (i++, i) += i++; // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  i += i+= 1; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i += i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i += ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i -= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i -= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i *= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i *= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i /= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i /= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i %= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i %= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i ^= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i ^= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i |= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i |= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i &= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i &= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i <<= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i <<= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i >>= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  i >>= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+
+  p[i++] = i; // cxx11-warning {{unsequenced modification and access to 'i'}}
+  p[i++] = (i = 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  p++[i++] = (i = p ? i++ : i++); // cxx11-warning {{unsequenced modification and access to 'p'}}
+                                  // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
 }
 
 namespace members {