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

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc2ec5dd20953: [clang-format] Left/Right alignment fixer can 
cause false positive replacements… (authored by MyDeveloperDay).

Repository:
  rG LLVM Github Monorepo

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 CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
 std::pair 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(
@@ -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 ,


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 CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
 std::pair 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(
@@ -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 ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[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 CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
 std::pair 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(
@@ -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 ,


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 CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
 std::pair 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(
@@ -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 ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/lib/Format/QualifierAlignmentFixer.cpp:95
+  if (Err)
+llvm::errs() << "Error removing no op replacements : "
+ << llvm::toString(std::move(Err)) << "\n";

The message may be confusing, but honestly I don't know what to put in either, 
since "adding non no op replacements" may be even more confusing.



Comment at: clang/unittests/Format/QualifierFixerTest.cpp:818
+  verifyFormat("static const uint32 foo[] = {0, 31};",
+   "static const uint32 foo[] = {0, 31};", Style);
+  EXPECT_EQ(ReplacementCount, 0);

Doesn't the two argument version suffice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110392

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 created this revision.
MyDeveloperDay added reviewers: curdeius, HazardyKnusperkeks.
MyDeveloperDay added projects: clang, clang-format.
MyDeveloperDay requested review of this revision.

Earlier during the development of D69764: [clang-format] Add Left/Right Const 
fixer capability  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.


Repository:
  rG LLVM Github Monorepo

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,18 @@
"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};",
+   "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 CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
 std::pair 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(
@@ -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 removing no op replacements : "
+ << llvm::toString(std::move(Err)) << "\n";
+}
+  }
+  return {NonNoOpFixes, 0};
 }
 
 static void replaceToken(const SourceManager ,


Index: clang/unittests/Format/QualifierFixerTest.cpp
===
--- clang/unittests/Format/QualifierFixerTest.cpp
+++ clang/unittests/Format/QualifierFixerTest.cpp
@@ -806,5 +806,18 @@
"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};",
+   "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 CurrentCode = None;
   tooling::Replacements Fixes;
-  unsigned Penalty = 0;
   for (size_t I = 0, E = Passes.size(); I < E; ++I) {
 std::pair PassFixes