[clang-tidy] Correctly handle evaluation order of designated initializers.

[mboehme]

Summary

As designated initializers show up only in the syntactic form of the
InitListExpr, we need to make sure we're searching both forms of the
InitListExpr when determining successors in the evaluation order.

This fixes a bug in bugprone-use-after-move where previously we erroneously
concluded that two designated initializers were unsequenced. The newly added
tests fail without the fix.

[PiotrZSL]

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

67

maybe some other name instead of allInitListExprForms, like getAllInitForms

69–72

this looks like typo... but it isn't, it's just unnecessary.

`InitList->isSemanticForm() && InitList->getSyntacticForm()` is equal to:
`AltForm.getInt() && AltForm.getPointer()`

`InitList->isSyntacticForm() && InitList->getSemanticForm()` is equal to:
`(!AltForm.getInt() || !AltForm.getPointer()) && (!AltForm.getInt()) && AltForm.getPointer()`

Why we coudn't have "getAnyForm".

any code could just look like this:
`if (const InitListExpr * Form = InitList->getSyntacticForm())`
` result.push_back(Form);`
`if (const InitListExpr * Form = InitList->getSemanticForm())`
` result.push_back(Form);`

[mboehme]

Changes in response to review comments.

[mboehme]

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

67

How about this?

69–72

this looks like typo... but it isn't, it's just unnecessary.

Ah, of course, thanks for pointing this out to me.

Why we coudn't have "getAnyForm".

Not sure exactly what you mean?

I've updated the code to do what you suggested above (i.e. don't call isSemanticForm(), just call getSyntacticForm() and see if returns non-null).

We still need to initialize result with InitList because InitList->getSyntacticForm() returns null if InitList is already in syntactic form.

I hope that makes sense? Please let me know if there's anything I missed from your original response.

[PiotrZSL]

Looks good to me, consider adding info to Release notes, that this check has been improved/fixed support for designated initializers.

[mboehme]

Added release notes

[mboehme]

In D145906#4193014, @PiotrZSL wrote:

Looks good to me, consider adding info to Release notes, that this check has been improved/fixed support for designated initializers.

Good point, done!

[PiotrZSL]

Regarding release-notes:
I would probably wrote something like "fixed handling for designated initializers", nonene will understand things like "sequence point".
If you decide to change this, the just commit this as an [NFC] without review.
And checks in this section should be in alphabetical order (so this one shouldn't be at the end), but that's a different story that can be corrected later for all those entrys.

[mboehme]

In D145906#4198583, @PiotrZSL wrote:

Regarding release-notes:
I would probably wrote something like "fixed handling for designated initializers", nonene will understand things like "sequence point".

Thanks, makes sense.

I've got another fix for the same checker in the pipeline anyway, so I've decided to roll this rewording into that patch: https://reviews.llvm.org/D145581

If you decide to change this, the just commit this as an [NFC] without review.
And checks in this section should be in alphabetical order (so this one shouldn't be at the end), but that's a different story that can be corrected later for all those entrys.

Right, I remember that too now...

As there are other entries that are also out of order, I'll just leave this where it is for now.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

 //===---------- ExprSequence.cpp - clang-tidy -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ExprSequence.h"
 #include "clang/AST/ParentMapContext.h"
+#include "llvm/ADT/SmallVector.h"
 #include <optional>
 
 namespace clang::tidy::utils {
 
 // Returns the Stmt nodes that are parents of 'S', skipping any potential
 // intermediate non-Stmt nodes.
 //
 // In almost all cases, this function returns a single parent or no parents at
if (const auto *S = Node.get<Stmt>()) {
       NodesToProcess.append(Parents.begin(), Parents.end());
     }
   }
 
   return Result;
 }
 
 namespace {
+
 bool isDescendantOrEqual(const Stmt *Descendant, const Stmt *Ancestor,
                          ASTContext *Context) {
   if (Descendant == Ancestor)
     return true;
   for (const Stmt *Parent : getParentStmts(Descendant, Context)) {
     if (isDescendantOrEqual(Parent, Ancestor, Context))
       return true;
   }
 
   return false;
 }
+
+llvm::SmallVector<const InitListExpr *>
+getAllInitListForms(const InitListExpr *InitList) {
+  llvm::SmallVector<const InitListExpr *> result = {InitList};
+  if (const InitListExpr *AltForm = InitList->getSyntacticForm())
+    result.push_back(AltForm);
+  if (const InitListExpr *AltForm = InitList->getSemanticForm())
+    result.push_back(AltForm);
+  return result;
+}
+
 } // namespace
 
 ExprSequence::ExprSequence(const CFG *TheCFG, const Stmt *Root,
                            ASTContext *TheContext)
     : Context(TheContext), Root(Root) {
   for (const auto &SyntheticStmt : TheCFG->synthetic_stmts()) {
     SyntheticStmtSourceMap[SyntheticStmt.first] = SyntheticStmt.second;
   }
for (const Stmt *Parent : getParentStmts(S, Context)) {
 
     if (const auto *BO = dyn_cast<BinaryOperator>(Parent)) {
       // Comma operator: Right-hand side is sequenced after the left-hand side.
       if (BO->getLHS() == S && BO->getOpcode() == BO_Comma)
         return BO->getRHS();
     } else if (const auto *InitList = dyn_cast<InitListExpr>(Parent)) {
       // Initializer list: Each initializer clause is sequenced after the
       // clauses that precede it.
-      for (unsigned I = 1; I < InitList->getNumInits(); ++I) {
-        if (InitList->getInit(I - 1) == S)
-          return InitList->getInit(I);
+      for (const InitListExpr *Form : getAllInitListForms(InitList)) {
+        for (unsigned I = 1; I < Form->getNumInits(); ++I) {
+          if (Form->getInit(I - 1) == S) {
+            return Form->getInit(I);
+          }
+        }
       }
     } else if (const auto *Compound = dyn_cast<CompoundStmt>(Parent)) {
       // Compound statement: Each sub-statement is sequenced after the
       // statements that precede it.
       const Stmt *Previous = nullptr;
       for (const auto *Child : Compound->body()) {
         if (Previous == S)
           return Child;

clang-tools-extra/docs/ReleaseNotes.rst

- Improved :doc:`readability-magic-numbers
   <clang-tidy/checks/readability/magic-numbers>` check, now allows for
   magic numbers in type aliases such as ``using`` and ``typedef`` declarations if
   the new ``IgnoreTypeAliases`` option is set to true.
 
 - Fixed a false positive in :doc:`cppcoreguidelines-slicing
   <clang-tidy/checks/cppcoreguidelines/slicing>` check when warning would be
   emitted in constructor for virtual base class initialization.
 
+- Improved :doc:`bugprone-use-after-move
+  <clang-tidy/checks/bugprone/use-after-move>` to understand that there is a
+  sequence point between designated initializers.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

 
 // An initializer list sequences its initialization clauses.
 void initializerListSequences() {
   {
     struct S1 {
       int i;
       A a;
     };
+    {
       A a;
       S1 s1{a.getInt(), std::move(a)};
     }
     {
+      A a;
+      S1 s1{.i = a.getInt(), .a = std::move(a)};
+    }
+  }
+  {
     struct S2 {
       A a;
       int i;
     };
+    {
       A a;
       S2 s2{std::move(a), a.getInt()};
-    // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
-    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+      // CHECK-NOTES: [[@LINE-1]]:27: warning: 'a' used after it was moved
+      // CHECK-NOTES: [[@LINE-2]]:13: note: move occurred here
+    }
+    {
+      A a;
+      S2 s2{.a = std::move(a), .i = a.getInt()};
+      // CHECK-NOTES: [[@LINE-1]]:37: warning: 'a' used after it was moved
+      // CHECK-NOTES: [[@LINE-2]]:13: note: move occurred here
+    }
   }
 }
 
 // A declaration statement containing multiple declarations sequences the
 // initializer expressions.
 void declarationSequences() {
   {
     A a;