[PATCH] D50766: Fix false positive unsequenced access and modification warning in array subscript expression.

[Mateusz Janek via Phabricator via cfe-commits]

stryku updated this revision to Diff 162585.
stryku added a comment.

Added test cases for the code.

Here I think I need your assistance. AFAIK in C++ 17 a couple more sequence related rules appeared. If we would like to cover them in current //warn-unsequenced.cpp// file, I think we would end up with a little mess with ifdefs for C++ < 17 and C++ >= 17. That's why I have introduced a new file //warn-unsequenced-cxx17.cpp// which covers my code and more tests will be added to it while their implementation as well. What do you think?


https://reviews.llvm.org/D50766

Files:
  lib/Sema/SemaChecking.cpp
  test/SemaCXX/warn-unsequenced-cxx17.cpp
  test/SemaCXX/warn-unsequenced.cpp


Index: test/SemaCXX/warn-unsequenced.cpp
===================================================================
--- test/SemaCXX/warn-unsequenced.cpp
+++ test/SemaCXX/warn-unsequenced.cpp
@@ -81,6 +81,7 @@
   int *p = xs;
   a = *(a++, p); // ok
   a = a++ && a; // ok
+  p[(long long unsigned)(p = 0)]; // expected-warning {{unsequenced modification and access to 'p'}}
 
   A *q = &agg1;
   (q = &agg2)->y = q->x; // expected-warning {{unsequenced modification and access to 'q'}}
Index: test/SemaCXX/warn-unsequenced-cxx17.cpp
===================================================================
--- /dev/null
+++ test/SemaCXX/warn-unsequenced-cxx17.cpp
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wno-unused %s
+
+void test() {
+  int xs[10];
+  int *p = xs;
+  // expected-no-diagnostics
+  p[(long long unsigned)(p = 0)] // ok
+}
Index: lib/Sema/SemaChecking.cpp
===================================================================
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -11658,30 +11658,42 @@
       notePostUse(O, E);
   }
 
-  void VisitBinComma(BinaryOperator *BO) {
-    // C++11 [expr.comma]p1:
-    //   Every value computation and side effect associated with the left
-    //   expression is sequenced before every value computation and side
-    //   effect associated with the right expression.
-    SequenceTree::Seq LHS = Tree.allocate(Region);
-    SequenceTree::Seq RHS = Tree.allocate(Region);
+  void VisitSequencedExpressions(Expr *SequencedBefore, Expr *SequencedAfter) {
+    SequenceTree::Seq BeforeRegion = Tree.allocate(Region);
+    SequenceTree::Seq AfterRegion = Tree.allocate(Region);
     SequenceTree::Seq OldRegion = Region;
 
     {
-      SequencedSubexpression SeqLHS(*this);
-      Region = LHS;
-      Visit(BO->getLHS());
+      SequencedSubexpression SeqBefore(*this);
+      Region = BeforeRegion;
+      Visit(SequencedBefore);
     }
 
-    Region = RHS;
-    Visit(BO->getRHS());
+    Region = AfterRegion;
+    Visit(SequencedAfter);
 
     Region = OldRegion;
 
-    // Forget that LHS and RHS are sequenced. They are both unsequenced
-    // with respect to other stuff.
-    Tree.merge(LHS);
-    Tree.merge(RHS);
+    Tree.merge(BeforeRegion);
+    Tree.merge(AfterRegion);
+  }
+
+  void VisitArraySubscriptExpr(ArraySubscriptExpr *ASE) {
+    // C++17 [expr.sub]p1:
+    //   The expression E1[E2] is identical (by definition) to *((E1)+(E2)). The
+    //   expression E1 is sequenced before the expression E2.
+    if (SemaRef.getLangOpts().CPlusPlus17)
+      VisitSequencedExpressions(ASE->getLHS(), ASE->getRHS());
+    else
+      Base::VisitStmt(ASE);
+  }
+
+  void VisitBinComma(BinaryOperator *BO) {
+    // C++11 [expr.comma]p1:
+    //   Every value computation and side effect associated with the left
+    //   expression is sequenced before every value computation and side
+    //   effect associated with the right expression.
+    VisitSequencedExpressions(BO->getLHS(), BO->getRHS());
   }
 
   void VisitBinAssign(BinaryOperator *BO) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D50766.162585.patch
Type: text/x-patch
Size: 3053 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180826/17b49470/attachment-0001.bin>