[clang-reorder-fields] Prevent rewriting unsupported cases #142149

vvuksanovic · 2025-05-30T13:42:34Z

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

github-actions · 2025-05-30T13:42:53Z

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 @ followed by their GitHub username.

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.

llvmbot · 2025-05-30T13:43:26Z

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

Author: Vladimir Vuksanovic (vvuksanovic)

Changes

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

Full diff: https://github.com/llvm/llvm-project/pull/142149.diff

4 Files Affected:

(modified) clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp (+51)
(added) clang-tools-extra/test/clang-reorder-fields/MacroExpandsToMultipleFields.cpp (+13)
(added) clang-tools-extra/test/clang-reorder-fields/MultipleFieldDeclsInStatement.cpp (+11)
(added) clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveInDefinition.cpp (+16)

diff --git a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp
index ea0207619fb2b..245da5e3433c5 100644
--- a/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp
+++ b/clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp
@@ -50,6 +50,55 @@ static const RecordDecl *findDefinition(StringRef RecordName,
   return selectFirst<RecordDecl>("recordDecl", Results);
 }
 
+static bool isSafeToRewrite(const RecordDecl *Decl, const ASTContext &Context) {
+  // Don't attempt to rewrite if there is a declaration like 'int a, b;'.
+  SourceLocation LastTypeLoc;
+  for (const auto &Field : Decl->fields()) {
+    SourceLocation TypeLoc =
+        Field->getTypeSourceInfo()->getTypeLoc().getBeginLoc();
+    if (LastTypeLoc.isValid() && TypeLoc == LastTypeLoc)
+      return false;
+    LastTypeLoc = TypeLoc;
+  }
+
+  // Don't attempt to rewrite if a single macro expansion creates multiple
+  // fields.
+  SourceLocation LastMacroLoc;
+  for (const auto &Field : Decl->fields()) {
+    if (!Field->getLocation().isMacroID())
+      continue;
+    SourceLocation MacroLoc =
+        Context.getSourceManager().getExpansionLoc(Field->getLocation());
+    if (LastMacroLoc.isValid() && MacroLoc == LastMacroLoc)
+      return false;
+    LastMacroLoc = MacroLoc;
+  }
+
+  // Skip if there are preprocessor directives present.
+  const SourceManager &SM = Context.getSourceManager();
+  std::pair<FileID, unsigned> FileAndOffset =
+      SM.getDecomposedLoc(Decl->getSourceRange().getBegin());
+  unsigned EndOffset = SM.getFileOffset(Decl->getSourceRange().getEnd());
+  StringRef SrcBuffer = SM.getBufferData(FileAndOffset.first);
+  Lexer L(SM.getLocForStartOfFile(FileAndOffset.first), Context.getLangOpts(),
+          SrcBuffer.data(), SrcBuffer.data() + FileAndOffset.second,
+          SrcBuffer.data() + SrcBuffer.size());
+  IdentifierTable Identifiers(Context.getLangOpts());
+  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 false;
+      }
+    }
+  }
+
+  return true;
+}
+
 /// Calculates the new order of fields.
 ///
 /// \returns empty vector if the list of fields doesn't match the definition.
@@ -341,6 +390,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/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

vvuksanovic · 2025-06-03T11:44:54Z

@alexander-shaposhnikov

clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveInDefinition.cpp

clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp

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

legrosbuffle · 2025-06-13T14:52:36Z

clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveAroundFields.cpp

+// This is okay to reorder.
+struct Foo {
+#ifdef DEFINE_FIELDS // CHECK:      {{^#ifdef DEFINE_FIELDS}}
+  int y;             // CHECK-NEXT: {{^ int y;}}


should this be:

int x; int y;

In the original file ?

Yes, good catch.

legrosbuffle · 2025-06-13T14:53:17Z

Not a big deal, but note for next time: we don't do force push to avoid losing context (https://llvm.org/docs/GitHub.html#rebasing-pull-requests-and-force-pushes)

github-actions · 2025-06-16T14:19:55Z

✅ With the latest revision this PR passed the C/C++ code formatter.

alexander-shaposhnikov · 2025-06-17T20:33:19Z

clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp

+  std::pair<FileID, unsigned> FileAndOffset =
+      SM.getDecomposedLoc(Decl->field_begin()->getBeginLoc());
+  auto LastField = Decl->field_begin();
+  while (std::next(LastField) != Decl->field_end())


why not std::prev(Decl->field_end()) ?

It is a forward iterator, std::prev doesn't work.

clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp

alexander-shaposhnikov · 2025-06-17T20:39:22Z

clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp

+    LastTypeLoc = TypeLoc;
+  }
+
+  // Don't attempt to rewrite if a single macro expansion creates multiple


LastMacroLoc is not used after this check,
perhaps, it'd be good to factor out this logic into a helper function (lines 70-80).

The same comment for lines 60-70

Refactored those and the preprocessor directive check, for consistency.

alexander-shaposhnikov

LG, thanks!

github-actions · 2025-06-23T02:00:29Z

@vvuksanovic Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

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

llvmbot added the clang-tools-extra label May 30, 2025

alexander-shaposhnikov requested review from alexander-shaposhnikov and legrosbuffle June 3, 2025 17:58

legrosbuffle reviewed Jun 4, 2025

View reviewed changes

clang-tools-extra/test/clang-reorder-fields/PreprocessorDirectiveInDefinition.cpp Show resolved Hide resolved

clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp Show resolved Hide resolved

vvuksanovic force-pushed the reorder-fields-unsupported branch from 7b4b942 to b8481a3 Compare June 9, 2025 07:35

vvuksanovic requested a review from legrosbuffle June 13, 2025 13:24

legrosbuffle reviewed Jun 13, 2025

View reviewed changes

fixup! [clang-reorder-fields] Prevent rewriting unsupported cases

e7bff07

fixup! [clang-reorder-fields] Prevent rewriting unsupported cases

6873c7f

alexander-shaposhnikov reviewed Jun 17, 2025

View reviewed changes

clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp Outdated Show resolved Hide resolved

alexander-shaposhnikov reviewed Jun 17, 2025

View reviewed changes

fixup! [clang-reorder-fields] Prevent rewriting unsupported cases

482b186

alexander-shaposhnikov approved these changes Jun 19, 2025

View reviewed changes

alexander-shaposhnikov merged commit a17b5bc into llvm:main Jun 23, 2025
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-reorder-fields] Prevent rewriting unsupported cases #142149

[clang-reorder-fields] Prevent rewriting unsupported cases #142149

Uh oh!

vvuksanovic commented May 30, 2025

Uh oh!

github-actions bot commented May 30, 2025

Uh oh!

llvmbot commented May 30, 2025

Uh oh!

vvuksanovic commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

legrosbuffle Jun 13, 2025

Uh oh!

vvuksanovic Jun 16, 2025

Uh oh!

legrosbuffle commented Jun 13, 2025

Uh oh!

github-actions bot commented Jun 16, 2025 •

edited

Loading

Uh oh!

alexander-shaposhnikov Jun 17, 2025

Uh oh!

vvuksanovic Jun 18, 2025

Uh oh!

Uh oh!

alexander-shaposhnikov Jun 17, 2025 •

edited

Loading

Uh oh!

vvuksanovic Jun 18, 2025

Uh oh!

alexander-shaposhnikov left a comment

Uh oh!

Uh oh!

github-actions bot commented Jun 23, 2025

Uh oh!

Uh oh!

[clang-reorder-fields] Prevent rewriting unsupported cases #142149

[clang-reorder-fields] Prevent rewriting unsupported cases #142149

Uh oh!

Conversation

vvuksanovic commented May 30, 2025

Uh oh!

github-actions bot commented May 30, 2025

Uh oh!

llvmbot commented May 30, 2025

Uh oh!

vvuksanovic commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

legrosbuffle Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

vvuksanovic Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

legrosbuffle commented Jun 13, 2025

Uh oh!

github-actions bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-shaposhnikov Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

vvuksanovic Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexander-shaposhnikov Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvuksanovic Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

alexander-shaposhnikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jun 23, 2025

Uh oh!

Uh oh!

github-actions bot commented Jun 16, 2025 •

edited

Loading

alexander-shaposhnikov Jun 17, 2025 •

edited

Loading