[clang-reorder-fields] Prevent rewriting unsupported cases

[vvuksanovic]

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]

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]

@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]

@alexander-shaposhnikov

[legrosbuffle]

Maybe add a test PreprocessorDirectiveAroundDefinition.cpp to make sure that we do reorder in cases like:

#ifndef STUFF
#define STUFF

struct Foo {
  int x;
  int y;
};

#endif

[legrosbuffle]

We could even start at the first field decl (and end at the last field decl) ? This should be fine:

struct S {
#ifdef STUFF
  int a;
  int b;
#endif
};