Skip to content

Commit 6538b43

Browse files
committed
[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
1 parent e951a48 commit 6538b43

File tree

5 files changed

+66
-7
lines changed

5 files changed

+66
-7
lines changed

clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -143,18 +143,26 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
143143
llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
144144
GroupedReplacements;
145145

146-
// Deduplicate identical replacements in diagnostics.
146+
// Deduplicate identical replacements in diagnostics unless they are from the
147+
// same TU.
147148
// FIXME: Find an efficient way to deduplicate on diagnostics level.
148-
llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement>>
149+
llvm::DenseMap<const FileEntry *,
150+
std::map<tooling::Replacement,
151+
const tooling::TranslationUnitDiagnostics *>>
149152
DiagReplacements;
150153

151-
auto AddToGroup = [&](const tooling::Replacement &R, bool FromDiag) {
154+
auto AddToGroup = [&](const tooling::Replacement &R,
155+
const tooling::TranslationUnitDiagnostics *SourceTU) {
152156
// Use the file manager to deduplicate paths. FileEntries are
153157
// automatically canonicalized.
154158
if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
155-
if (FromDiag) {
159+
if (SourceTU) {
156160
auto &Replaces = DiagReplacements[*Entry];
157-
if (!Replaces.insert(R).second)
161+
auto It = Replaces.find(R);
162+
if (It == Replaces.end())
163+
Replaces.emplace(R, SourceTU);
164+
else if (It->second != SourceTU)
165+
// This replacement is a duplicate of one suggested by another TU.
158166
return;
159167
}
160168
GroupedReplacements[*Entry].push_back(R);
@@ -166,14 +174,14 @@ groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
166174

167175
for (const auto &TU : TUs)
168176
for (const tooling::Replacement &R : TU.Replacements)
169-
AddToGroup(R, false);
177+
AddToGroup(R, nullptr);
170178

171179
for (const auto &TU : TUDs)
172180
for (const auto &D : TU.Diagnostics)
173181
if (const auto *ChoosenFix = tooling::selectFirstFix(D)) {
174182
for (const auto &Fix : *ChoosenFix)
175183
for (const tooling::Replacement &R : Fix.second)
176-
AddToGroup(R, true);
184+
AddToGroup(R, &TU);
177185
}
178186

179187
// Sort replacements per file to keep consistent behavior when
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
MainSourceFile: identical_in_TU.cpp
3+
Diagnostics:
4+
- DiagnosticName: test-identical-insertion
5+
DiagnosticMessage:
6+
Message: Fix
7+
FilePath: $(path)/identical_in_TU.cpp
8+
FileOffset: 12
9+
Replacements:
10+
- FilePath: $(path)/identical_in_TU.cpp
11+
Offset: 12
12+
Length: 0
13+
ReplacementText: '0'
14+
- FilePath: $(path)/identical_in_TU.cpp
15+
Offset: 12
16+
Length: 0
17+
ReplacementText: '0'
18+
...
19+
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
MainSourceFile: identical-in-TU.cpp
3+
Diagnostics:
4+
- DiagnosticName: test-identical-insertion
5+
DiagnosticMessage:
6+
Message: Fix
7+
FilePath: $(path)/identical-in-TU.cpp
8+
FileOffset: 12
9+
Replacements:
10+
- FilePath: $(path)/identical-in-TU.cpp
11+
Offset: 12
12+
Length: 0
13+
ReplacementText: '0'
14+
- FilePath: $(path)/identical-in-TU.cpp
15+
Offset: 12
16+
Length: 0
17+
ReplacementText: '0'
18+
...
19+
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
class MyType {};
2+
// CHECK: class MyType00 {};
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// RUN: mkdir -p %T/Inputs/identical-in-TU
2+
3+
// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical-in-TU/identical-in-TU.cpp > %T/Inputs/identical-in-TU/identical-in-TU.cpp
4+
// RUN: sed "s#\$(path)#%/T/Inputs/identical-in-TU#" %S/Inputs/identical-in-TU/file1.yaml > %T/Inputs/identical-in-TU/file1.yaml
5+
// RUN: sed "s#\$(path)#%/T/Inputs/identical-in-TU#" %S/Inputs/identical-in-TU/file2.yaml > %T/Inputs/identical-in-TU/file2.yaml
6+
// RUN: clang-apply-replacements %T/Inputs/identical-in-TU
7+
// RUN: FileCheck -input-file=%T/Inputs/identical-in-TU/identical-in-TU.cpp %S/Inputs/identical-in-TU/identical-in-TU.cpp
8+
9+
// Similar to identical test but each yaml file contains the same fix twice.
10+
// This check ensures that only the duplicated replacements in a single yaml
11+
// file are applied twice. Addresses PR45150.

0 commit comments

Comments
 (0)