[clang-tools-extra] [clang-reorder-fields] Support designated initializers (PR #142150)

via cfe-commits cfe-commits at lists.llvm.org
Fri May 30 06:44:26 PDT 2025


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Vladimir Vuksanovic (vvuksanovic)

<details>
<summary>Changes</summary>

Initializer lists with designators, missing elements or omitted braces can now be rewritten. Any missing designators are added and they get sorted according to the new order.

```
struct Foo {
  int a;
  int b;
  int c;
};
struct Foo foo = { .a = 1, 2, 3 }
```

when reordering elements to "b,a,c" becomes:

```
struct Foo {
  int b;
  int a;
  int c;
};
struct Foo foo = { .b = 2, .a = 1, .c = 3 }
```

---

Patch is 34.11 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/142150.diff


5 Files Affected:

- (modified) clang-tools-extra/clang-reorder-fields/CMakeLists.txt (+1) 
- (modified) clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp (+253-48) 
- (added) clang-tools-extra/clang-reorder-fields/utils/Designator.cpp (+256) 
- (added) clang-tools-extra/clang-reorder-fields/utils/Designator.h (+118) 
- (added) clang-tools-extra/test/clang-reorder-fields/DesignatedInitializerList.c (+31) 


``````````diff
diff --git a/clang-tools-extra/clang-reorder-fields/CMakeLists.txt b/clang-tools-extra/clang-reorder-fields/CMakeLists.txt
index 2fdeb65d89767..dfb28234fd548 100644
--- a/clang-tools-extra/clang-reorder-fields/CMakeLists.txt
+++ b/clang-tools-extra/clang-reorder-fields/CMakeLists.txt
@@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
 )
 
 add_clang_library(clangReorderFields STATIC
+  utils/Designator.cpp
   ReorderFieldsAction.cpp
 
   DEPENDS
diff --git a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp
index ea0207619fb2b..f5961a7dab0c9 100644
--- a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp
+++ b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp
@@ -13,6 +13,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "ReorderFieldsAction.h"
+#include "utils/Designator.h"
 #include "clang/AST/AST.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
@@ -23,6 +24,7 @@
 #include "clang/Tooling/Refactoring.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/Support/ErrorHandling.h"
 #include <string>
 
 namespace clang {
@@ -81,7 +83,44 @@ getNewFieldsOrder(const RecordDecl *Definition,
   return NewFieldsOrder;
 }
 
+struct ReorderedStruct {
+public:
+  ReorderedStruct(const RecordDecl *Decl, ArrayRef<unsigned> NewFieldsOrder)
+      : Definition(Decl), NewFieldsOrder(NewFieldsOrder),
+        NewFieldsPositions(NewFieldsOrder.size()) {
+    for (unsigned I = 0; I < NewFieldsPositions.size(); ++I)
+      NewFieldsPositions[NewFieldsOrder[I]] = I;
+  }
+
+  const RecordDecl *Definition;
+  ArrayRef<unsigned> NewFieldsOrder;
+  SmallVector<unsigned, 4> NewFieldsPositions;
+};
+
 // FIXME: error-handling
+/// Replaces a range of source code by the specified text.
+static void
+addReplacement(SourceRange Old, StringRef New, const ASTContext &Context,
+               std::map<std::string, tooling::Replacements> &Replacements) {
+  tooling::Replacement R(Context.getSourceManager(),
+                         CharSourceRange::getTokenRange(Old), New,
+                         Context.getLangOpts());
+  consumeError(Replacements[std::string(R.getFilePath())].add(R));
+}
+
+/// Replaces one range of source code by another and adds a prefix.
+static void
+addReplacement(SourceRange Old, SourceRange New, StringRef Prefix,
+               const ASTContext &Context,
+               std::map<std::string, tooling::Replacements> &Replacements) {
+  std::string NewText =
+      (Prefix + Lexer::getSourceText(CharSourceRange::getTokenRange(New),
+                                     Context.getSourceManager(),
+                                     Context.getLangOpts()))
+          .str();
+  addReplacement(Old, NewText, Context, Replacements);
+}
+
 /// Replaces one range of source code by another.
 static void
 addReplacement(SourceRange Old, SourceRange New, const ASTContext &Context,
@@ -89,10 +128,7 @@ addReplacement(SourceRange Old, SourceRange New, const ASTContext &Context,
   StringRef NewText =
       Lexer::getSourceText(CharSourceRange::getTokenRange(New),
                            Context.getSourceManager(), Context.getLangOpts());
-  tooling::Replacement R(Context.getSourceManager(),
-                         CharSourceRange::getTokenRange(Old), NewText,
-                         Context.getLangOpts());
-  consumeError(Replacements[std::string(R.getFilePath())].add(R));
+  addReplacement(Old, NewText.str(), Context, Replacements);
 }
 
 /// Find all member fields used in the given init-list initializer expr
@@ -194,33 +230,33 @@ static SourceRange getFullFieldSourceRange(const FieldDecl &Field,
 /// different accesses (public/protected/private) is not supported.
 /// \returns true on success.
 static bool reorderFieldsInDefinition(
-    const RecordDecl *Definition, ArrayRef<unsigned> NewFieldsOrder,
-    const ASTContext &Context,
+    const ReorderedStruct &RS, const ASTContext &Context,
     std::map<std::string, tooling::Replacements> &Replacements) {
-  assert(Definition && "Definition is null");
+  assert(RS.Definition && "Definition is null");
 
   SmallVector<const FieldDecl *, 10> Fields;
-  for (const auto *Field : Definition->fields())
+  for (const auto *Field : RS.Definition->fields())
     Fields.push_back(Field);
 
   // Check that the permutation of the fields doesn't change the accesses
-  for (const auto *Field : Definition->fields()) {
+  for (const auto *Field : RS.Definition->fields()) {
     const auto FieldIndex = Field->getFieldIndex();
-    if (Field->getAccess() != Fields[NewFieldsOrder[FieldIndex]]->getAccess()) {
+    if (Field->getAccess() !=
+        Fields[RS.NewFieldsOrder[FieldIndex]]->getAccess()) {
       llvm::errs() << "Currently reordering of fields with different accesses "
                       "is not supported\n";
       return false;
     }
   }
 
-  for (const auto *Field : Definition->fields()) {
+  for (const auto *Field : RS.Definition->fields()) {
     const auto FieldIndex = Field->getFieldIndex();
-    if (FieldIndex == NewFieldsOrder[FieldIndex])
+    if (FieldIndex == RS.NewFieldsOrder[FieldIndex])
       continue;
-    addReplacement(
-        getFullFieldSourceRange(*Field, Context),
-        getFullFieldSourceRange(*Fields[NewFieldsOrder[FieldIndex]], Context),
-        Context, Replacements);
+    addReplacement(getFullFieldSourceRange(*Field, Context),
+                   getFullFieldSourceRange(
+                       *Fields[RS.NewFieldsOrder[FieldIndex]], Context),
+                   Context, Replacements);
   }
   return true;
 }
@@ -231,7 +267,7 @@ static bool reorderFieldsInDefinition(
 /// fields. Thus, we need to ensure that we reorder just the initializers that
 /// are present.
 static void reorderFieldsInConstructor(
-    const CXXConstructorDecl *CtorDecl, ArrayRef<unsigned> NewFieldsOrder,
+    const CXXConstructorDecl *CtorDecl, const ReorderedStruct &RS,
     ASTContext &Context,
     std::map<std::string, tooling::Replacements> &Replacements) {
   assert(CtorDecl && "Constructor declaration is null");
@@ -243,10 +279,6 @@ static void reorderFieldsInConstructor(
   // Thus this assert needs to be after the previous checks.
   assert(CtorDecl->isThisDeclarationADefinition() && "Not a definition");
 
-  SmallVector<unsigned, 10> NewFieldsPositions(NewFieldsOrder.size());
-  for (unsigned i = 0, e = NewFieldsOrder.size(); i < e; ++i)
-    NewFieldsPositions[NewFieldsOrder[i]] = i;
-
   SmallVector<const CXXCtorInitializer *, 10> OldWrittenInitializersOrder;
   SmallVector<const CXXCtorInitializer *, 10> NewWrittenInitializersOrder;
   for (const auto *Initializer : CtorDecl->inits()) {
@@ -257,8 +289,8 @@ static void reorderFieldsInConstructor(
     const FieldDecl *ThisM = Initializer->getMember();
     const auto UsedMembers = findMembersUsedInInitExpr(Initializer, Context);
     for (const FieldDecl *UM : UsedMembers) {
-      if (NewFieldsPositions[UM->getFieldIndex()] >
-          NewFieldsPositions[ThisM->getFieldIndex()]) {
+      if (RS.NewFieldsPositions[UM->getFieldIndex()] >
+          RS.NewFieldsPositions[ThisM->getFieldIndex()]) {
         DiagnosticsEngine &DiagEngine = Context.getDiagnostics();
         auto Description = ("reordering field " + UM->getName() + " after " +
                             ThisM->getName() + " makes " + UM->getName() +
@@ -276,8 +308,8 @@ static void reorderFieldsInConstructor(
   auto ByFieldNewPosition = [&](const CXXCtorInitializer *LHS,
                                 const CXXCtorInitializer *RHS) {
     assert(LHS && RHS);
-    return NewFieldsPositions[LHS->getMember()->getFieldIndex()] <
-           NewFieldsPositions[RHS->getMember()->getFieldIndex()];
+    return RS.NewFieldsPositions[LHS->getMember()->getFieldIndex()] <
+           RS.NewFieldsPositions[RHS->getMember()->getFieldIndex()];
   };
   llvm::sort(NewWrittenInitializersOrder, ByFieldNewPosition);
   assert(OldWrittenInitializersOrder.size() ==
@@ -289,35 +321,205 @@ static void reorderFieldsInConstructor(
                      Replacements);
 }
 
+/// Replacement for broken InitListExpr::isExplicit function.
+/// TODO: Remove when InitListExpr::isExplicit is fixed.
+static bool isImplicitILE(const InitListExpr *ILE, const ASTContext &Context) {
+  // The ILE is implicit if either:
+  // - The left brace loc of the ILE matches the start of first init expression
+  //   (for non designated decls)
+  // - The right brace loc of the ILE matches the end of first init expression
+  //   (for designated decls)
+  // The first init expression should be taken from the syntactic form, but
+  // since the ILE could be implicit, there might not be a syntactic form.
+  // For that reason we have to check against all init expressions.
+  for (const Expr *Init : ILE->inits()) {
+    if (ILE->getLBraceLoc() == Init->getBeginLoc() ||
+        ILE->getRBraceLoc() == Init->getEndLoc())
+      return true;
+  }
+  return false;
+}
+
+/// Compares compatible designators according to the new struct order.
+/// Returns a negative value if Lhs < Rhs, positive value if Lhs > Rhs and 0 if
+/// they are equal.
+static int cmpDesignators(const DesignatorIter &Lhs, const DesignatorIter &Rhs,
+                          const ReorderedStruct &Struct) {
+  assert(Lhs.getTag() == Rhs.getTag() && "Incompatible designators");
+  switch (Lhs.getTag()) {
+  case DesignatorIter::STRUCT: {
+    assert(Lhs.getStructDecl() == Rhs.getStructDecl() &&
+           "Incompatible structs");
+    // Use the new layout for reordered struct.
+    if (Struct.Definition == Lhs.getStructDecl()) {
+      return Struct.NewFieldsPositions[Lhs.getStructIter()->getFieldIndex()] -
+             Struct.NewFieldsPositions[Rhs.getStructIter()->getFieldIndex()];
+    }
+    return Lhs.getStructIter()->getFieldIndex() -
+           Rhs.getStructIter()->getFieldIndex();
+  }
+  case DesignatorIter::ARRAY:
+    return Lhs.getArrayIndex() - Rhs.getArrayIndex();
+  case DesignatorIter::ARRAY_RANGE:
+    return Lhs.getArrayRangeEnd() - Rhs.getArrayRangeEnd();
+  }
+  llvm_unreachable("Invalid designator tag");
+}
+
+/// Compares compatible designator lists according to the new struct order.
+/// Returns a negative value if Lhs < Rhs, positive value if Lhs > Rhs and 0 if
+/// they are equal.
+static int cmpDesignatorLists(const Designators &Lhs, const Designators &Rhs,
+                              const ReorderedStruct &Reorders) {
+  for (unsigned Idx = 0, Size = std::min(Lhs.size(), Rhs.size()); Idx < Size;
+       ++Idx) {
+    int DesignatorComp = cmpDesignators(Lhs[Idx], Rhs[Idx], Reorders);
+    // If the current designators are not equal, return the result
+    if (DesignatorComp != 0)
+      return DesignatorComp;
+    // Otherwise, continue to the next pair.
+  }
+  //
+  return Lhs.size() - Rhs.size();
+}
+
+/// Finds the semantic form of the first explicit ancestor of the given
+/// initializer list including itself.
+static const InitListExpr *getExplicitILE(const InitListExpr *ILE,
+                                          ASTContext &Context) {
+  if (!isImplicitILE(ILE, Context))
+    return ILE;
+  const InitListExpr *TopLevelILE = ILE;
+  DynTypedNodeList Parents = Context.getParents(*TopLevelILE);
+  while (!Parents.empty() && Parents.begin()->get<InitListExpr>()) {
+    TopLevelILE = Parents.begin()->get<InitListExpr>();
+    Parents = Context.getParents(*TopLevelILE);
+    if (!isImplicitILE(TopLevelILE, Context))
+      break;
+  }
+  if (!TopLevelILE->isSemanticForm()) {
+    return TopLevelILE->getSemanticForm();
+  }
+  return TopLevelILE;
+}
+
 /// Reorders initializers in the brace initialization of an aggregate.
 ///
 /// At the moment partial initialization is not supported.
 /// \returns true on success
 static bool reorderFieldsInInitListExpr(
-    const InitListExpr *InitListEx, ArrayRef<unsigned> NewFieldsOrder,
-    const ASTContext &Context,
+    const InitListExpr *InitListEx, const ReorderedStruct &RS,
+    ASTContext &Context,
     std::map<std::string, tooling::Replacements> &Replacements) {
   assert(InitListEx && "Init list expression is null");
-  // We care only about InitListExprs which originate from source code.
-  // Implicit InitListExprs are created by the semantic analyzer.
-  if (!InitListEx->isExplicit())
+  // Only process semantic forms of initializer lists.
+  if (!InitListEx->isSemanticForm()) {
     return true;
-  // The method InitListExpr::getSyntacticForm may return nullptr indicating
-  // that the current initializer list also serves as its syntactic form.
-  if (const auto *SyntacticForm = InitListEx->getSyntacticForm())
-    InitListEx = SyntacticForm;
+  }
+
   // If there are no initializers we do not need to change anything.
   if (!InitListEx->getNumInits())
     return true;
-  if (InitListEx->getNumInits() != NewFieldsOrder.size()) {
-    llvm::errs() << "Currently only full initialization is supported\n";
-    return false;
+
+  // We care only about InitListExprs which originate from source code.
+  // Implicit InitListExprs are created by the semantic analyzer.
+  // We find the first parent InitListExpr that exists in source code and
+  // process it. This is necessary because of designated initializer lists and
+  // possible omitted braces.
+  InitListEx = getExplicitILE(InitListEx, Context);
+
+  // Find if there are any designated initializations or implicit values. If all
+  // initializers are present and none have designators then just reorder them
+  // normally. Otherwise, designators are added to all initializers and they are
+  // sorted in the new order.
+  bool ShouldAddDesignators = false;
+  // The method InitListExpr::getSyntacticForm may return nullptr indicating
+  // that the current initializer list also serves as its syntactic form.
+  const InitListExpr *SyntacticInitListEx = InitListEx;
+  if (const InitListExpr *SynILE = InitListEx->getSyntacticForm()) {
+    // Do not rewrite zero initializers. This check is only valid for syntactic
+    // forms.
+    if (SynILE->isIdiomaticZeroInitializer(Context.getLangOpts()))
+      return true;
+
+    ShouldAddDesignators = InitListEx->getNumInits() != SynILE->getNumInits() ||
+                           llvm::any_of(SynILE->inits(), [](const Expr *Init) {
+                             return isa<DesignatedInitExpr>(Init);
+                           });
+
+    SyntacticInitListEx = SynILE;
+  } else {
+    // If there is no syntactic form, there can be no designators. Instead,
+    // there might be implicit values.
+    ShouldAddDesignators =
+        (RS.NewFieldsOrder.size() != InitListEx->getNumInits()) ||
+        llvm::any_of(InitListEx->inits(), [&Context](const Expr *Init) {
+          return isa<ImplicitValueInitExpr>(Init) ||
+                 (isa<InitListExpr>(Init) &&
+                  isImplicitILE(dyn_cast<InitListExpr>(Init), Context));
+        });
+  }
+
+  if (ShouldAddDesignators) {
+    // Designators are only supported from C++20.
+    if (Context.getLangOpts().CPlusPlus && !Context.getLangOpts().CPlusPlus20) {
+      llvm::errs()
+          << "Currently only full initialization is supported for C++\n";
+      return false;
+    }
+
+    // Handle case when some fields are designated. Some fields can be
+    // missing. Insert any missing designators and reorder the expressions
+    // according to the new order.
+    Designators CurrentDesignators{};
+    // Remember each initializer expression along with its designators. They are
+    // sorted later to determine the correct order.
+    std::vector<std::pair<Designators, const Expr *>> Rewrites;
+    for (const Expr *Init : SyntacticInitListEx->inits()) {
+      if (const auto *DIE = dyn_cast_or_null<DesignatedInitExpr>(Init)) {
+        CurrentDesignators = {DIE, SyntacticInitListEx, Context};
+
+        // Use the child of the DesignatedInitExpr. This way designators are
+        // always replaced.
+        Rewrites.push_back({CurrentDesignators, DIE->getInit()});
+      } else {
+        // Find the next field.
+        if (!CurrentDesignators.increment(SyntacticInitListEx, Init, Context)) {
+          llvm::errs() << "Unsupported initializer list\n";
+          return false;
+        }
+
+        // Do not rewrite implicit values. They just had to be processed to
+        // find the correct designator.
+        if (!isa<ImplicitValueInitExpr>(Init))
+          Rewrites.push_back({CurrentDesignators, Init});
+      }
+    }
+
+    // Sort the designators according to the new order.
+    llvm::sort(Rewrites, [&RS](const auto &Lhs, const auto &Rhs) {
+      return cmpDesignatorLists(Lhs.first, Rhs.first, RS) < 0;
+    });
+
+    for (unsigned i = 0, e = Rewrites.size(); i < e; ++i) {
+      addReplacement(SyntacticInitListEx->getInit(i)->getSourceRange(),
+                     Rewrites[i].second->getSourceRange(),
+                     Rewrites[i].first.toString(), Context, Replacements);
+    }
+  } else {
+    // Handle excess initializers by leaving them unchanged.
+    assert(SyntacticInitListEx->getNumInits() >= InitListEx->getNumInits());
+
+    // All field initializers are present and none have designators. They can be
+    // reordered normally.
+    for (unsigned i = 0, e = RS.NewFieldsOrder.size(); i < e; ++i) {
+      if (i != RS.NewFieldsOrder[i])
+        addReplacement(SyntacticInitListEx->getInit(i)->getSourceRange(),
+                       SyntacticInitListEx->getInit(RS.NewFieldsOrder[i])
+                           ->getSourceRange(),
+                       Context, Replacements);
+    }
   }
-  for (unsigned i = 0, e = InitListEx->getNumInits(); i < e; ++i)
-    if (i != NewFieldsOrder[i])
-      addReplacement(InitListEx->getInit(i)->getSourceRange(),
-                     InitListEx->getInit(NewFieldsOrder[i])->getSourceRange(),
-                     Context, Replacements);
   return true;
 }
 
@@ -345,7 +547,9 @@ class ReorderingConsumer : public ASTConsumer {
         getNewFieldsOrder(RD, DesiredFieldsOrder);
     if (NewFieldsOrder.empty())
       return;
-    if (!reorderFieldsInDefinition(RD, NewFieldsOrder, Context, Replacements))
+    ReorderedStruct RS{RD, NewFieldsOrder};
+
+    if (!reorderFieldsInDefinition(RS, Context, Replacements))
       return;
 
     // CXXRD will be nullptr if C code (not C++) is being processed.
@@ -353,24 +557,25 @@ class ReorderingConsumer : public ASTConsumer {
     if (CXXRD)
       for (const auto *C : CXXRD->ctors())
         if (const auto *D = dyn_cast<CXXConstructorDecl>(C->getDefinition()))
-          reorderFieldsInConstructor(cast<const CXXConstructorDecl>(D),
-                                     NewFieldsOrder, Context, Replacements);
+          reorderFieldsInConstructor(cast<const CXXConstructorDecl>(D), RS,
+                                     Context, Replacements);
 
     // We only need to reorder init list expressions for
     // plain C structs or C++ aggregate types.
     // For other types the order of constructor parameters is used,
     // which we don't change at the moment.
     // Now (v0) partial initialization is not supported.
-    if (!CXXRD || CXXRD->isAggregate())
+    if (!CXXRD || CXXRD->isAggregate()) {
       for (auto Result :
            match(initListExpr(hasType(equalsNode(RD))).bind("initListExpr"),
                  Context))
         if (!reorderFieldsInInitListExpr(
-                Result.getNodeAs<InitListExpr>("initListExpr"), NewFieldsOrder,
-                Context, Replacements)) {
+                Result.getNodeAs<InitListExpr>("initListExpr"), RS, Context,
+                Replacements)) {
           Replacements.clear();
           return;
         }
+    }
   }
 };
 } // end anonymous namespace
diff --git a/clang-tools-extra/clang-reorder-fields/utils/Designator.cpp b/clang-tools-extra/clang-reorder-fields/utils/Designator.cpp
new file mode 100644
index 0000000000000..9ad60a3fc5db6
--- /dev/null
+++ b/clang-tools-extra/clang-reorder-fields/utils/Designator.cpp
@@ -0,0 +1,256 @@
+//===-- tools/extra/clang-reorder-fields/utils/Designator.cpp ---*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------...
[truncated]

``````````

</details>


https://github.com/llvm/llvm-project/pull/142150


More information about the cfe-commits mailing list