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

[via cfe-commits]

Author: mydeveloperday
Date: 2021-09-25T17:35:41+01:00
New Revision: c2ec5dd209532b1d618958ade6a7d550a0c31ea5

URL: https://github.com/llvm/llvm-project/commit/c2ec5dd209532b1d618958ade6a7d550a0c31ea5
DIFF: https://github.com/llvm/llvm-project/commit/c2ec5dd209532b1d618958ade6a7d550a0c31ea5.diff

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

Earlier during the development of {D69764} I felt it was no longer necessary to
ensure we were not trying to change code which didn't need to change
and we felt this could be removed, however I'd like to bring this back for now
as I am seeing some false positives in terms of the "replacements"

What I see is the generation of a replacement which is a "No Op" on the original
code, I think this comes about because of the merging of replacements:

```
static const a;
->
const static a;
->
static const a;
```

The replacements don't really merge, in such a way as to identify when we have gone
back to the original

Also remove the Penalty as I'm not using it (and it became marked as set and no used,
I'd rather get rid of it if it means nothing)

I think we need to do this step for now, as many people use the --output-replacements-xml
to identify that the file "needs a clang-format"

The same can be seen with the -n or --dry-run option as this uses the replacements
to drive the error/warning output.

Reviewed By: HazardyKnusperkeks

Differential Revision: https://reviews.llvm.org/D110392

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang/lib/Format/QualifierAlignmentFixer.cpp b/clang/lib/Format/QualifierAlignmentFixer.cpp
index 4a42442ca24bc..30b199952edbd 100644
--- a/clang/lib/Format/QualifierAlignmentFixer.cpp
+++ b/clang/lib/Format/QualifierAlignmentFixer.cpp
@@ -67,14 +67,12 @@ std::pair<tooling::Replacements, unsigned> QualifierAlignmentFixer::analyze(
                                     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 @@ std::pair<tooling::Replacements, unsigned> QualifierAlignmentFixer::analyze(
       }
     }
   }
-  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,

diff  --git a/clang/unittests/Format/QualifierFixerTest.cpp b/clang/unittests/Format/QualifierFixerTest.cpp
index 0592cef1eaae5..e3d0a9a0e2607 100755
--- a/clang/unittests/Format/QualifierFixerTest.cpp
+++ b/clang/unittests/Format/QualifierFixerTest.cpp
@@ -806,5 +806,17 @@ TEST_F(QualifierFixerTest, UnsignedQualifier) {
                "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