[PATCH] D110392: [clang-format] Left/Right alignment fixer can cause false positive replacements when they don't actually change anything

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay updated this revision to Diff 375023.
MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.

Address review comment


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110392/new/

https://reviews.llvm.org/D110392

Files:
  clang/lib/Format/QualifierAlignmentFixer.cpp
  clang/unittests/Format/QualifierFixerTest.cpp


Index: clang/unittests/Format/QualifierFixerTest.cpp
===================================================================
--- clang/unittests/Format/QualifierFixerTest.cpp
+++ clang/unittests/Format/QualifierFixerTest.cpp
@@ -806,5 +806,17 @@
                "Foo(unsigned const char *bytes)", Style);
 }
 
+TEST_F(QualifierFixerTest, NoOpQualifierReplacements) {
+
+  FormatStyle Style = getLLVMStyle();
+  Style.QualifierAlignment = FormatStyle::QAS_Custom;
+  Style.QualifierOrder = {"static", "const", "type"};
+
+  ReplacementCount = 0;
+  EXPECT_EQ(ReplacementCount, 0);
+  verifyFormat("static const uint32 foo[] = {0, 31};", Style);
+  EXPECT_EQ(ReplacementCount, 0);
+}
+
 } // namespace format
 } // namespace clang
Index: clang/lib/Format/QualifierAlignmentFixer.cpp
===================================================================
--- clang/lib/Format/QualifierAlignmentFixer.cpp
+++ clang/lib/Format/QualifierAlignmentFixer.cpp
@@ -67,14 +67,12 @@
                                     NextStartColumn, LastStartColumn);
   llvm::Optional<std::string> CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
     std::pair<tooling::Replacements, unsigned> PassFixes = Passes[I](*Env);
     auto NewCode = applyAllReplacements(
         CurrentCode ? StringRef(*CurrentCode) : Code, PassFixes.first);
     if (NewCode) {
       Fixes = Fixes.merge(PassFixes.first);
-      Penalty += PassFixes.second;
       if (I + 1 < E) {
         CurrentCode = std::move(*NewCode);
         Env = std::make_unique<Environment>(
@@ -84,7 +82,21 @@
       }
     }
   }
-  return {Fixes, Penalty};
+
+  // Don't make replacements that replace nothing.
+  tooling::Replacements NonNoOpFixes;
+
+  for (auto I = Fixes.begin(), E = Fixes.end(); I != E; ++I) {
+    StringRef OriginalCode = Code.substr(I->getOffset(), I->getLength());
+
+    if (!OriginalCode.equals(I->getReplacementText())) {
+      auto Err = NonNoOpFixes.add(*I);
+      if (Err)
+        llvm::errs() << "Error adding replacements : "
+                     << llvm::toString(std::move(Err)) << "\n";
+    }
+  }
+  return {NonNoOpFixes, 0};
 }
 
 static void replaceToken(const SourceManager &SourceMgr,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D110392.375023.patch
Type: text/x-patch
Size: 2248 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210925/4643ac2d/attachment.bin>