[clang-tools-extra] 6538b43 - [clang-apply-replacements] No longer deduplucates replacements from the same TU
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Wed Mar 25 02:36:48 PDT 2020
Author: Nathan James
Date: 2020-03-25T09:36:36Z
New Revision: 6538b4393dc3e7df9fee2b07eba148d4cf603a24
URL: https://github.com/llvm/llvm-project/commit/6538b4393dc3e7df9fee2b07eba148d4cf603a24
DIFF: https://github.com/llvm/llvm-project/commit/6538b4393dc3e7df9fee2b07eba148d4cf603a24.diff
LOG: [clang-apply-replacements] No longer deduplucates replacements from the same TU
Summary:
clang-apply-replacements currently deduplicates all diagnostic replacements. However if you get a duplicated replacement from one TU then its expected that it should not be deduplicated. This goes some way to solving [[ https://bugs.llvm.org/show_bug.cgi?id=45150 | export-fixes to yaml adds extra newlines and breaks offsets. ]]
Take this example yaml.
```
---
MainSourceFile: '/home/nathan/test/test.cpp'
Diagnostics:
- DiagnosticName: readability-braces-around-statements
DiagnosticMessage:
Message: statement should be inside braces
FilePath: '/home/nathan/test/test.cpp'
FileOffset: 14
Replacements:
- FilePath: '/home/nathan/test/test.cpp'
Offset: 14
Length: 0
ReplacementText: ' {'
- FilePath: '/home/nathan/test/test.cpp'
Offset: 28
Length: 0
ReplacementText: '
}'
- DiagnosticName: readability-braces-around-statements
DiagnosticMessage:
Message: statement should be inside braces
FilePath: '/home/nathan/test/test.cpp'
FileOffset: 20
Replacements:
- FilePath: '/home/nathan/test/test.cpp'
Offset: 20
Length: 0
ReplacementText: ' {'
- FilePath: '/home/nathan/test/test.cpp'
Offset: 28
Length: 0
ReplacementText: '
}'
...```
The current behaviour is to deduplicate the text insertions at Offset 28 and only apply one of the replacements.
However as both of these replacements came from the same translation unit we can be confident they were both meant to be applied together
The new behaviour won't deduplicate the text insertion and instead insert both of the replacements.
If the duplicate replacement is found inside different translation units (from a header file change perhaps) then they will still be deduplicated as before.
Reviewers: aaron.ballman, gribozavr2, klimek, ymandel
Reviewed By: ymandel
Subscribers: ymandel, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D76054
Added:
clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file1.yaml
clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file2.yaml
clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp
clang-tools-extra/test/clang-apply-replacements/identical-in-TU.cpp
Modified:
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp b/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
index 33ece7f1b4e0..1e5012b9891b 100644
--- a/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ b/clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -143,18 +143,26 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
GroupedReplacements;
- // Deduplicate identical replacements in diagnostics.
+ // Deduplicate identical replacements in diagnostics unless they are from the
+ // same TU.
// FIXME: Find an efficient way to deduplicate on diagnostics level.
- llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement>>
+ llvm::DenseMap<const FileEntry *,
+ std::map<tooling::Replacement,
+ const tooling::TranslationUnitDiagnostics *>>
DiagReplacements;
- auto AddToGroup = [&](const tooling::Replacement &R, bool FromDiag) {
+ auto AddToGroup = [&](const tooling::Replacement &R,
+ const tooling::TranslationUnitDiagnostics *SourceTU) {
// Use the file manager to deduplicate paths. FileEntries are
// automatically canonicalized.
if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
- if (FromDiag) {
+ if (SourceTU) {
auto &Replaces = DiagReplacements[*Entry];
- if (!Replaces.insert(R).second)
+ auto It = Replaces.find(R);
+ if (It == Replaces.end())
+ Replaces.emplace(R, SourceTU);
+ else if (It->second != SourceTU)
+ // This replacement is a duplicate of one suggested by another TU.
return;
}
GroupedReplacements[*Entry].push_back(R);
@@ -166,14 +174,14 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
for (const auto &TU : TUs)
for (const tooling::Replacement &R : TU.Replacements)
- AddToGroup(R, false);
+ AddToGroup(R, nullptr);
for (const auto &TU : TUDs)
for (const auto &D : TU.Diagnostics)
if (const auto *ChoosenFix = tooling::selectFirstFix(D)) {
for (const auto &Fix : *ChoosenFix)
for (const tooling::Replacement &R : Fix.second)
- AddToGroup(R, true);
+ AddToGroup(R, &TU);
}
// Sort replacements per file to keep consistent behavior when
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file1.yaml b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file1.yaml
new file mode 100644
index 000000000000..65a1b47a175c
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file1.yaml
@@ -0,0 +1,19 @@
+---
+MainSourceFile: identical_in_TU.cpp
+Diagnostics:
+ - DiagnosticName: test-identical-insertion
+ DiagnosticMessage:
+ Message: Fix
+ FilePath: $(path)/identical_in_TU.cpp
+ FileOffset: 12
+ Replacements:
+ - FilePath: $(path)/identical_in_TU.cpp
+ Offset: 12
+ Length: 0
+ ReplacementText: '0'
+ - FilePath: $(path)/identical_in_TU.cpp
+ Offset: 12
+ Length: 0
+ ReplacementText: '0'
+...
+
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file2.yaml b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file2.yaml
new file mode 100644
index 000000000000..5297e974dec4
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file2.yaml
@@ -0,0 +1,19 @@
+---
+MainSourceFile: identical-in-TU.cpp
+Diagnostics:
+ - DiagnosticName: test-identical-insertion
+ DiagnosticMessage:
+ Message: Fix
+ FilePath: $(path)/identical-in-TU.cpp
+ FileOffset: 12
+ Replacements:
+ - FilePath: $(path)/identical-in-TU.cpp
+ Offset: 12
+ Length: 0
+ ReplacementText: '0'
+ - FilePath: $(path)/identical-in-TU.cpp
+ Offset: 12
+ Length: 0
+ ReplacementText: '0'
+...
+
diff --git a/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp
new file mode 100644
index 000000000000..bdaab4fc823a
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp
@@ -0,0 +1,2 @@
+class MyType {};
+// CHECK: class MyType00 {};
diff --git a/clang-tools-extra/test/clang-apply-replacements/identical-in-TU.cpp b/clang-tools-extra/test/clang-apply-replacements/identical-in-TU.cpp
new file mode 100644
index 000000000000..024db114b589
--- /dev/null
+++ b/clang-tools-extra/test/clang-apply-replacements/identical-in-TU.cpp
@@ -0,0 +1,11 @@
+// RUN: mkdir -p %T/Inputs/identical-in-TU
+
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical-in-TU/identical-in-TU.cpp > %T/Inputs/identical-in-TU/identical-in-TU.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/identical-in-TU#" %S/Inputs/identical-in-TU/file1.yaml > %T/Inputs/identical-in-TU/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/identical-in-TU#" %S/Inputs/identical-in-TU/file2.yaml > %T/Inputs/identical-in-TU/file2.yaml
+// RUN: clang-apply-replacements %T/Inputs/identical-in-TU
+// RUN: FileCheck -input-file=%T/Inputs/identical-in-TU/identical-in-TU.cpp %S/Inputs/identical-in-TU/identical-in-TU.cpp
+
+// Similar to identical test but each yaml file contains the same fix twice.
+// This check ensures that only the duplicated replacements in a single yaml
+// file are applied twice. Addresses PR45150.
More information about the cfe-commits
mailing list