[clang-tools-extra] [clang-reorder-fields] Prevent rewriting unsupported cases (PR #142149)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Vladimir Vuksanovic (vvuksanovic)

<details>
<summary>Changes</summary>

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

``````````

</details>


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