[clang-reorder-fields] Support designated initializers #142150
+667
−48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tools-extra Author: Vladimir Vuksanovic (vvuksanovic) ChangesInitializer 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. when reordering elements to "b,a,c" becomes: 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:
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]
|
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 }
```
vvuksanovic
force-pushed
the
reorder-fields-designators
branch
from
67febc3 to
df2e394
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
when reordering elements to "b,a,c" becomes: