Author: Vladimir Vuksanovic Date: 2025-06-22T19:00:11-07:00 New Revision: a17b5bce8c9bfa7767827df392855cff9f2af372
URL: https://github.com/llvm/llvm-project/commit/a17b5bce8c9bfa7767827df392855cff9f2af372 DIFF: https://github.com/llvm/llvm-project/commit/a17b5bce8c9bfa7767827df392855cff9f2af372.diff LOG: [clang-reorder-fields] Prevent rewriting unsupported cases (#142149) Add checks to prevent rewriting when doing so might result in incorrect code. The following cases are checked: - There are multiple field declarations in one statement like `int a, b` - Multiple fields are created from a single macro expansion - Preprocessor directives are present in the struct Added: clang-tools-extra/test/clang-reorder-fields/MacroExpandsToMultipleFields.cpp clang-tools-extra/test/clang-reorder-fields/MultipleFieldDeclsInStatement.cpp clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundDefinition.cpp clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundFields.cpp clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveInDefinition.cpp Modified: clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp index 3b1cd18d80346..ada9122b587ae 100644 --- a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp +++ b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp @@ -19,6 +19,8 @@ #include "clang/AST/Decl.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/LangOptions.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Lex/Lexer.h" #include "clang/Tooling/Refactoring.h" #include "llvm/ADT/STLExtras.h" @@ -50,6 +52,85 @@ static const RecordDecl *findDefinition(StringRef RecordName, return selectFirst<RecordDecl>("recordDecl", Results); } +static bool declaresMultipleFieldsInStatement(const RecordDecl *Decl) { + SourceLocation LastTypeLoc; + for (const auto &Field : Decl->fields()) { + SourceLocation TypeLoc = + Field->getTypeSourceInfo()->getTypeLoc().getBeginLoc(); + if (LastTypeLoc.isValid() && TypeLoc == LastTypeLoc) + return true; + LastTypeLoc = TypeLoc; + } + return false; +} + +static bool declaresMultipleFieldsInMacro(const RecordDecl *Decl, + const SourceManager &SrcMgr) { + SourceLocation LastMacroLoc; + for (const auto &Field : Decl->fields()) { + if (!Field->getLocation().isMacroID()) + continue; + SourceLocation MacroLoc = SrcMgr.getExpansionLoc(Field->getLocation()); + if (LastMacroLoc.isValid() && MacroLoc == LastMacroLoc) + return true; + LastMacroLoc = MacroLoc; + } + return false; +} + +static bool containsPreprocessorDirectives(const RecordDecl *Decl, + const SourceManager &SrcMgr, + const LangOptions &LangOpts) { + std::pair<FileID, unsigned> FileAndOffset = + SrcMgr.getDecomposedLoc(Decl->field_begin()->getBeginLoc()); + assert(!Decl->field_empty()); + auto LastField = Decl->field_begin(); + while (std::next(LastField) != Decl->field_end()) + ++LastField; + unsigned EndOffset = SrcMgr.getFileOffset(LastField->getEndLoc()); + StringRef SrcBuffer = SrcMgr.getBufferData(FileAndOffset.first); + Lexer L(SrcMgr.getLocForStartOfFile(FileAndOffset.first), LangOpts, + SrcBuffer.data(), SrcBuffer.data() + FileAndOffset.second, + SrcBuffer.data() + SrcBuffer.size()); + IdentifierTable Identifiers(LangOpts); + clang::Token T; + while (!L.LexFromRawLexer(T) && L.getCurrentBufferOffset() < EndOffset) { + if (T.getKind() == tok::hash) { + L.LexFromRawLexer(T); + if (T.getKind() == tok::raw_identifier) { + clang::IdentifierInfo &II = Identifiers.get(T.getRawIdentifier()); + if (II.getPPKeywordID() != clang::tok::pp_not_keyword) + return true; + } + } + } + return false; +} + +static bool isSafeToRewrite(const RecordDecl *Decl, const ASTContext &Context) { + // All following checks expect at least one field declaration. + if (Decl->field_empty()) + return true; + + // Don't attempt to rewrite if there is a declaration like 'int a, b;'. + if (declaresMultipleFieldsInStatement(Decl)) + return false; + + const SourceManager &SrcMgr = Context.getSourceManager(); + + // Don't attempt to rewrite if a single macro expansion creates multiple + // fields. + if (declaresMultipleFieldsInMacro(Decl, SrcMgr)) + return false; + + // Prevent rewriting if there are preprocessor directives present between the + // start of the first field and the end of last field. + if (containsPreprocessorDirectives(Decl, SrcMgr, Context.getLangOpts())) + return false; + + return true; +} + /// Calculates the new order of fields. /// /// \returns empty vector if the list of fields doesn't match the definition. @@ -345,6 +426,8 @@ class ReorderingConsumer : public ASTConsumer { const RecordDecl *RD = findDefinition(RecordName, Context); if (!RD) return; + if (!isSafeToRewrite(RD, Context)) + return; SmallVector<unsigned, 4> NewFieldsOrder = getNewFieldsOrder(RD, DesiredFieldsOrder); if (NewFieldsOrder.empty()) diff --git a/clang-tools-extra/test/clang-reorder-fields/MacroExpandsToMultipleFields.cpp b/clang-tools-extra/test/clang-reorder-fields/MacroExpandsToMultipleFields.cpp new file mode 100644 index 0000000000000..5bafcd19ea829 --- /dev/null +++ b/clang-tools-extra/test/clang-reorder-fields/MacroExpandsToMultipleFields.cpp @@ -0,0 +1,13 @@ +// RUN: clang-reorder-fields -record-name ::bar::Foo -fields-order z,y,x %s -- | FileCheck %s + +namespace bar { + +#define FIELDS_DECL int x; int y; // CHECK: {{^#define FIELDS_DECL int x; int y;}} + +// The order of fields should not change. +struct Foo { + FIELDS_DECL // CHECK: {{^ FIELDS_DECL}} + int z; // CHECK-NEXT: {{^ int z;}} +}; + +} // end namespace bar diff --git a/clang-tools-extra/test/clang-reorder-fields/MultipleFieldDeclsInStatement.cpp b/clang-tools-extra/test/clang-reorder-fields/MultipleFieldDeclsInStatement.cpp new file mode 100644 index 0000000000000..437e7b91e27a3 --- /dev/null +++ b/clang-tools-extra/test/clang-reorder-fields/MultipleFieldDeclsInStatement.cpp @@ -0,0 +1,11 @@ +// RUN: clang-reorder-fields -record-name ::bar::Foo -fields-order z,y,x %s -- | FileCheck %s + +namespace bar { + +// The order of fields should not change. +struct Foo { + int x, y; // CHECK: {{^ int x, y;}} + double z; // CHECK-NEXT: {{^ double z;}} +}; + +} // end namespace bar diff --git a/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundDefinition.cpp b/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundDefinition.cpp new file mode 100644 index 0000000000000..f00b4b0b57bf7 --- /dev/null +++ b/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundDefinition.cpp @@ -0,0 +1,15 @@ +// RUN: clang-reorder-fields -record-name ::bar::Foo -fields-order y,x %s -- | FileCheck %s + +namespace bar { + +#define DEFINE_FOO + +// This is okay to reorder. +#ifdef DEFINE_FOO +struct Foo { + int x; // CHECK: {{^ int y;}} + int y; // CHECK-NEXT: {{^ int x;}} +}; +#endif + +} // end namespace bar diff --git a/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundFields.cpp b/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundFields.cpp new file mode 100644 index 0000000000000..c37546a05afd3 --- /dev/null +++ b/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundFields.cpp @@ -0,0 +1,15 @@ +// RUN: clang-reorder-fields -record-name ::bar::Foo -fields-order y,x %s -- | FileCheck %s + +namespace bar { + +#define DEFINE_FIELDS + +// This is okay to reorder. +struct Foo { +#ifdef DEFINE_FIELDS // CHECK: {{^#ifdef DEFINE_FIELDS}} + int x; // CHECK-NEXT: {{^ int y;}} + int y; // CHECK-NEXT: {{^ int x;}} +#endif // CHECK-NEXT: {{^#endif}} +}; + +} // end namespace bar diff --git a/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveInDefinition.cpp b/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveInDefinition.cpp new file mode 100644 index 0000000000000..fee6b0e637b9b --- /dev/null +++ b/clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveInDefinition.cpp @@ -0,0 +1,16 @@ +// RUN: clang-reorder-fields -record-name ::bar::Foo -fields-order z,y,x %s -- | FileCheck %s + +namespace bar { + +#define ADD_Z + +// The order of fields should not change. +struct Foo { + int x; // CHECK: {{^ int x;}} + int y; // CHECK-NEXT: {{^ int y;}} +#ifdef ADD_Z // CHECK-NEXT: {{^#ifdef ADD_Z}} + int z; // CHECK-NEXT: {{^ int z;}} +#endif // CHECK-NEXT: {{^#endif}} +}; + +} // end namespace bar _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
