Fix false positive unsequenced access and modification warning in array subscript expression.

[stryku]

Summary

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

[hiraditya]

lib/Sema/SemaChecking.cpp

11676–11677

Is this comment still relevant?

[stryku]

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

[Rakete1111]

Your patch breaks a lot of stuff in the test suite. For example:

void f() {
  int A = 0;
  (A++, A) = 1; // warning from this patch, but this is perfectly valid since forever.
}

Also, you don't take into account the fact that the rule you mention was added in C++17. In versions prior to that (and C!), the warning is not a false positive.

[stryku]

@Rakete1111 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.

[lebedev.ri]

Test coverage?
Also, please always upload all patches with full context (-U99999)

[stryku]

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

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?

EDIT
Sorry, I've found the tests. I'll cover my implementation there.

[stryku]

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?

[Rakete1111]

What do you think?

Good idea!

test/SemaCXX/warn-unsequenced-cxx17.cpp

7 ↗

(On Diff #162585)

Oh no, you forgot a semicolon there! :)

[stryku]

Added missing semicolon.

[stryku]

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

test/SemaCXX/warn-unsequenced-cxx17.cpp

7 ↗

(On Diff #162585)

Sorry. I remember that I had this problem and I've resolved it. Apparently I've sent wrong patch file.

[stryku]

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?

[stryku]

Friendly ping (:

[stryku]

@Rakete1111 Any more comments?

[Rakete1111]

Sorry, for some reason I didn't see your updates.

Can you add a test for C++17? Then your patch is good to go! :)

[Rakete1111]

Nevermind my last comment, I was tired. LGTM

[rsmith]

Thanks, LGTM!

Are you interested in fixing the other cases for which p0145 tightened evaluation order (., ->, .*, ->*, <<, >>, callee in a function call, assignment and compound assignment) too? =)

[lebedev.ri]

test/SemaCXX/warn-unsequenced-cxx17.cpp

1 ↗

(On Diff #162630)

One last-minute thought: this is only a positive test.
You don't test what happens before C++17.

[Rakete1111]

test/SemaCXX/warn-unsequenced-cxx17.cpp

1 ↗

(On Diff #162630)

It is tested. Look at the diff for test/SemaCXX/warn-unsequenced.cpp :)
Or are you suggesting to merge the two files?

[lebedev.ri]

test/SemaCXX/warn-unsequenced-cxx17.cpp

1 ↗

(On Diff #162630)

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.

[stryku]

test/SemaCXX/warn-unsequenced-cxx17.cpp

1 ↗

(On Diff #162630)

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+.

[stryku]

@rsmith Of course. 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)?

[rsmith]

In D50766#1262460, @stryku wrote:

@rsmith Of course. 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)?

It's really up to you. I don't think a patch that does them all would be too big to review, and doing N small reviews isn't really much more work than one larger review, so please go with whichever approach best suits your workflow.

[stryku]

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

lib/Sema/SemaChecking.cpp

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