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

[Mateusz Janek via Phabricator via cfe-commits]

stryku created this revision.
stryku added a reviewer: rsmith.
stryku added a project: clang.

In the [expr.sub] p1, we can read that for a given E1[E2], E1 is sequenced 
before E2.


Repository:
  rC Clang

https://reviews.llvm.org/D50766

Files:
  lib/Sema/SemaChecking.cpp


Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -11658,30 +11658,40 @@
   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 VisitSequencedLhsRhsExpressions(Expr *LHS, Expr *RHS) {
+SequenceTree::Seq LHSRegion = Tree.allocate(Region);
+SequenceTree::Seq RHSRegion = Tree.allocate(Region);
 SequenceTree::Seq OldRegion = Region;
 
 {
   SequencedSubexpression SeqLHS(*this);
-  Region = LHS;
-  Visit(BO->getLHS());
+  Region = LHSRegion;
+  Visit(LHS);
 }
 
-Region = RHS;
-Visit(BO->getRHS());
+Region = RHSRegion;
+Visit(RHS);
 
 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(LHSRegion);
+Tree.merge(RHSRegion);
+  }
+
+  void VisitArraySubscriptExpr(ArraySubscriptExpr *ASE) {
+// The expression E1[E2] is identical (by definition) to *((E1)+(E2)). The
+// expression E1 is sequenced before the expression E2.
+VisitSequencedLhsRhsExpressions(ASE->getLHS(), ASE->getLHS());
+  }
+
+  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.
+VisitSequencedLhsRhsExpressions(BO->getLHS(), BO->getLHS());
   }
 
   void VisitBinAssign(BinaryOperator *BO) {


Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -11658,30 +11658,40 @@
   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 VisitSequencedLhsRhsExpressions(Expr *LHS, Expr *RHS) {
+SequenceTree::Seq LHSRegion = Tree.allocate(Region);
+SequenceTree::Seq RHSRegion = Tree.allocate(Region);
 SequenceTree::Seq OldRegion = Region;
 
 {
   SequencedSubexpression SeqLHS(*this);
-  Region = LHS;
-  Visit(BO->getLHS());
+  Region = LHSRegion;
+  Visit(LHS);
 }
 
-Region = RHS;
-Visit(BO->getRHS());
+Region = RHSRegion;
+Visit(RHS);
 
 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(LHSRegion);
+Tree.merge(RHSRegion);
+  }
+
+  void VisitArraySubscriptExpr(ArraySubscriptExpr *ASE) {
+// The expression E1[E2] is identical (by definition) to *((E1)+(E2)). The
+// expression E1 is sequenced before the expression E2.
+VisitSequencedLhsRhsExpressions(ASE->getLHS(), ASE->getLHS());
+  }
+
+  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.
+VisitSequencedLhsRhsExpressions(BO->getLHS(), BO->getLHS());
   }
 
   void VisitBinAssign(BinaryOperator *BO) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 160885.
stryku added a comment.

Thanks for pointing that out. You're probably right, these two calls are 
self-explanatory.


https://reviews.llvm.org/D50766

Files:
  lib/Sema/SemaChecking.cpp


Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -11658,30 +11658,38 @@
   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 VisitSequencedLhsRhsExpressions(Expr *LHS, Expr *RHS) {
+SequenceTree::Seq LHSRegion = Tree.allocate(Region);
+SequenceTree::Seq RHSRegion = Tree.allocate(Region);
 SequenceTree::Seq OldRegion = Region;
 
 {
   SequencedSubexpression SeqLHS(*this);
-  Region = LHS;
-  Visit(BO->getLHS());
+  Region = LHSRegion;
+  Visit(LHS);
 }
 
-Region = RHS;
-Visit(BO->getRHS());
+Region = RHSRegion;
+Visit(RHS);
 
 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(LHSRegion);
+Tree.merge(RHSRegion);
+  }
+
+  void VisitArraySubscriptExpr(ArraySubscriptExpr *ASE) {
+// The expression E1[E2] is identical (by definition) to *((E1)+(E2)). The
+// expression E1 is sequenced before the expression E2.
+VisitSequencedLhsRhsExpressions(ASE->getLHS(), ASE->getLHS());
+  }
+
+  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.
+VisitSequencedLhsRhsExpressions(BO->getLHS(), BO->getLHS());
   }
 
   void VisitBinAssign(BinaryOperator *BO) {


Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -11658,30 +11658,38 @@
   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 VisitSequencedLhsRhsExpressions(Expr *LHS, Expr *RHS) {
+SequenceTree::Seq LHSRegion = Tree.allocate(Region);
+SequenceTree::Seq RHSRegion = Tree.allocate(Region);
 SequenceTree::Seq OldRegion = Region;
 
 {
   SequencedSubexpression SeqLHS(*this);
-  Region = LHS;
-  Visit(BO->getLHS());
+  Region = LHSRegion;
+  Visit(LHS);
 }
 
-Region = RHS;
-Visit(BO->getRHS());
+Region = RHSRegion;
+Visit(RHS);
 
 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(LHSRegion);
+Tree.merge(RHSRegion);
+  }
+
+  void VisitArraySubscriptExpr(ArraySubscriptExpr *ASE) {
+// The expression E1[E2] is identical (by definition) to *((E1)+(E2)). The
+// expression E1 is sequenced before the expression E2.
+VisitSequencedLhsRhsExpressions(ASE->getLHS(), ASE->getLHS());
+  }
+
+  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.
+VisitSequencedLhsRhsExpressions(BO->getLHS(), BO->getLHS());
   }
 
   void VisitBinAssign(BinaryOperator *BO) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 161943.
stryku added a comment.

@Rakete Thank you for the comments. You're perfectly right, there was an 
issue in my code. I've fixed it and also E1 and E2 will be sequenced only in 
C++ >= 17.


https://reviews.llvm.org/D50766

Files:
  lib/Sema/SemaChecking.cpp


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) {


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) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku added a comment.

Thanks for the advice. Should I update a new diff with `-U9`?

About tests. I've probably missed something but I looked through the project 
and didn't find any tests for unsequenced operations. If you know a place where 
such tests are placed, please point it and I'll add the one for this code.
If there are no tests for unsequenced operations, should I introduce them?


https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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) {


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
-//   expressio

[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 162630.
stryku added a comment.

Added missing semicolon.


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) {


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

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku marked 2 inline comments as done.
stryku added a comment.

@Rakete or anyone else, any more comments? (:




Comment at: test/SemaCXX/warn-unsequenced-cxx17.cpp:7
+  // expected-no-diagnostics
+  p[(long long unsigned)(p = 0)] // ok
+}

Rakete wrote:
> Oh no, you forgot a semicolon there! :)
Sorry. I remember that I had this problem and I've resolved it. Apparently I've 
sent wrong patch file.


https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku marked an inline comment as done.
stryku added a comment.

Not sure if you guys are still reviewing this. Do you have any more thoughts? 
If no, what should I do to move further with the patch?


https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku marked an inline comment as done.
stryku added inline comments.



Comment at: test/SemaCXX/warn-unsequenced-cxx17.cpp:1
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++17 -Wno-unused %s
+

lebedev.ri wrote:
> Rakete wrote:
> > lebedev.ri wrote:
> > > One last-minute thought: this is only a positive test.
> > > You don't test what happens before C++17.
> > It is tested. Look at the diff for test/SemaCXX/warn-unsequenced.cpp :)
> > Or are you suggesting to merge the two files?
> I see that the negative test is in `warn-unsequenced.cpp`, but the positive 
> test is in `warn-unsequenced-cxx17.cpp`.
> This split is the reason of my remark.
> I'm not sure if this is an issue, or if merging them is the solution.
Like I said in some of the previous comments, merging these files, AFAIU, would 
introduce a little ifdefing. With other sequence-related topics from p0145 
there would be a lot more ifdefing introduced, and IMHO the file would be a lot 
less readable. That's why I've introduced this file for C++17+.


https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku added a comment.

@rsmith Of course I would like. Not sure what's better: to submit one patch per 
case or submit all of them in one patch (didn't deal much with them, not sure 
how big will the change be)?


https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku added a comment.

Friendly ping (:


https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku added a comment.

@Rakete Any more comments?


https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mateusz Janek via Phabricator via cfe-commits]

stryku added a comment.

Hi again, since I don't have commit rights, could you please merge this patch 
into the project sources? Thanks in advance.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50766/new/

https://reviews.llvm.org/D50766



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29039: Proposal for clang-format -r option

[Mateusz Janek via Phabricator via cfe-commits]

stryku added a comment.

@djasper You're probably right. Thanks anyway for your time (:


Repository:
  rL LLVM

https://reviews.llvm.org/D29039



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits