[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6538b4393dc3: [clang-apply-replacements] No longer 
deduplucates replacements from the same TU (authored by njames93).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  
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

Index: clang-tools-extra/test/clang-apply-replacements/identical-in-TU.cpp
===
--- /dev/null
+++ 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.
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp
@@ -0,0 +1,2 @@
+class MyType {};
+// CHECK: class MyType00 {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file2.yaml
===
--- /dev/null
+++ 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'
+...
+
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file1.yaml
===
--- /dev/null
+++ 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'
+...
+
Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -143,18 +143,26 @@
   llvm::DenseMap>
   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>
+  llvm::DenseMap>
   DiagReplacements;
 
-  auto AddToGroup = [&](const tooling::Replacement , bool FromDiag) {
+  auto AddToGroup = [&](const tooling::Replacement ,
+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  = DiagReplacements[*Entry];
-if (!Replaces.insert(R).second)

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 252017.
njames93 marked 5 inline comments as done.
njames93 added a comment.

- Address reviewer comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  
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

Index: clang-tools-extra/test/clang-apply-replacements/identical-in-TU.cpp
===
--- /dev/null
+++ 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.
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/identical-in-TU.cpp
@@ -0,0 +1,2 @@
+class MyType {};
+// CHECK: class MyType00 {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file2.yaml
===
--- /dev/null
+++ 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'
+...
+
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical-in-TU/file1.yaml
===
--- /dev/null
+++ 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'
+...
+
Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -143,18 +143,26 @@
   llvm::DenseMap>
   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>
+  llvm::DenseMap>
   DiagReplacements;
 
-  auto AddToGroup = [&](const tooling::Replacement , bool FromDiag) {
+  auto AddToGroup = [&](const tooling::Replacement ,
+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  = DiagReplacements[*Entry];
-if (!Replaces.insert(R).second)
+auto It = Replaces.find(R);
+if (It == 

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/test/clang-apply-replacements/identical2.cpp:3
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical2/identical2.cpp > 
%T/Inputs/identical2/identical2.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/identical2#" 
%S/Inputs/identical2/file1.yaml > %T/Inputs/identical2/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/identical2#" 
%S/Inputs/identical2/file2.yaml > %T/Inputs/identical2/file2.yaml

ymandel wrote:
> Why is this `%/T` rather than `%T`? I realize it is the same in 
> `identical.cpp`, but just want to understand what I'm reading...
It's like that in most of the checks, not sure why but wanted to keep it 
consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.

Thanks for the example and, generally, explanation. Just a few small comments.




Comment at: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:148
   // FIXME: Find an efficient way to deduplicate on diagnostics level.
-  llvm::DenseMap>
+  llvm::DenseMap 
%T/Inputs/identical2/identical2.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/identical2#" 
%S/Inputs/identical2/file1.yaml > %T/Inputs/identical2/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/identical2#" 
%S/Inputs/identical2/file2.yaml > %T/Inputs/identical2/file2.yaml

Why is this `%/T` rather than `%T`? I realize it is the same in 
`identical.cpp`, but just want to understand what I'm reading...



Comment at: clang-tools-extra/test/clang-apply-replacements/identical2.cpp:8
+
+// 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 

Consider differentiating the name of this test more and making it more 
descriptive, e.g. identical_in_TU. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D76054#1935533 , @ymandel wrote:

> Thanks for expanding the description, including the helpful example.  I'm not 
> sure, though, that this is the "right" behavior, at least not always. Worse, 
> I'm not sure there is a single "right" behavior. I can easily imagine a tidy 
> that matches multiple times in the same TU and tries, therefore, to apply the 
> same fix multiple times, even though it wants at most one (and possibly an 
> error).  The major problem is that character-level (versus AST level) edits 
> simply don't compose. So, multiple edits on a file are always suspect.  Could 
> you also give an example (in terms of code changes, not YAML) of why this 
> comes up? As in, are we sure that the problem lies in the algorithm here, 
> rather than in the phrasing of the transformation itself?
>
> That said, based on some of your linked bugs, it sounds like your change 
> would make the behavior here consistent with the behavior in another 
> situation (when clang-tidy applies the edits directly rather than outputting 
> to YAML?).  Consistency could be a strong argument in favor of this change.


clang-tidy does a good job of filtering out conflicting fix-its so duplicated 
insertions are going to be intentional.

The example yaml is generated from running is running the 
readability-braces-around-statements check on this code(offsets dont match due 
to formatting)

  void f() {
if (1)
  if (2) return;
  }

Both if statements have the same end location which is where the check will 
insert a `}`. 
This leads to 2 insertion fix its that are the same location and same text. 
When clang-tidy applies the fix-it there is no issue, but currently 
clang-apply-replacements thinks they should be deduplicated. 
This ultimately leads to malformed code as 2 `{` are inserted after the if 
conditions, but only one `}` is inserted at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Thanks for expanding the description, including the helpful example.  I'm not 
sure, though, that this is the "right" behavior, at least not always. Worse, 
I'm not sure there is a single "right" behavior. I can easily imagine a tidy 
that matches multiple times in the same TU and tries, therefore, to apply the 
same fix multiple times, even though it wants at most one (and possibly an 
error).  The major problem is that character-level (versus AST level) edits 
simply don't compose. So, multiple edits on a file are always suspect.  Could 
you also give an example (in terms of code changes, not YAML) of why this comes 
up? As in, are we sure that the problem lies in the algorithm here, rather than 
in the phrasing of the transformation itself?

That said, based on some of your linked bugs, it sounds like your change would 
make the behavior here consistent with the behavior in another situation (when 
clang-tidy applies the edits directly rather than outputting to YAML?).  
Consistency could be a strong argument in favor of this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

Can you please expand on the description? Specifically, can you clarify what 
you've *changed* in this patch? I gather that previously, it distinguished 
between two different sources of replacements: diagnostics and otherwise, and 
only deduplicated the ones from diagnostics.  What is the new behavior?

Also, perhaps give a simple example in the description?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Unfortunately I cannot say whether the code is good, but the fix works and 
certainly helps readability-braces-around-statements which can sometimes add 
multiple } at the same offset to close several scopes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 250054.
njames93 added a comment.

- Added test cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/file1.yaml
  clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/file2.yaml
  
clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/identical2.cpp
  clang-tools-extra/test/clang-apply-replacements/identical2.cpp

Index: clang-tools-extra/test/clang-apply-replacements/identical2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/identical2.cpp
@@ -0,0 +1,10 @@
+// RUN: mkdir -p %T/Inputs/identical2
+// RUN: grep -Ev "// *[A-Z-]+:" %S/Inputs/identical2/identical2.cpp > %T/Inputs/identical2/identical2.cpp
+// RUN: sed "s#\$(path)#%/T/Inputs/identical2#" %S/Inputs/identical2/file1.yaml > %T/Inputs/identical2/file1.yaml
+// RUN: sed "s#\$(path)#%/T/Inputs/identical2#" %S/Inputs/identical2/file2.yaml > %T/Inputs/identical2/file2.yaml
+// RUN: clang-apply-replacements %T/Inputs/identical2
+// RUN: FileCheck -input-file=%T/Inputs/identical2/identical2.cpp %S/Inputs/identical2/identical2.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.
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/identical2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/identical2.cpp
@@ -0,0 +1,2 @@
+class MyType {};
+// CHECK: class MyType00 {};
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/file2.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/file2.yaml
@@ -0,0 +1,19 @@
+---
+MainSourceFile: identical2.cpp
+Diagnostics:
+  - DiagnosticName: test-identical-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/identical2.cpp
+  FileOffset: 12
+  Replacements:
+- FilePath:$(path)/identical2.cpp
+  Offset:  12
+  Length:  0
+  ReplacementText: '0'
+- FilePath:$(path)/identical2.cpp
+  Offset:  12
+  Length:  0
+  ReplacementText: '0'
+...
+
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/file1.yaml
===
--- /dev/null
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/identical2/file1.yaml
@@ -0,0 +1,19 @@
+---
+MainSourceFile: identical2.cpp
+Diagnostics:
+  - DiagnosticName: test-identical-insertion
+DiagnosticMessage:
+  Message: Fix
+  FilePath: $(path)/identical2.cpp
+  FileOffset: 12
+  Replacements:
+- FilePath:$(path)/identical2.cpp
+  Offset:  12
+  Length:  0
+  ReplacementText: '0'
+- FilePath:$(path)/identical2.cpp
+  Offset:  12
+  Length:  0
+  ReplacementText: '0'
+...
+
Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -145,16 +145,21 @@
 
   // Deduplicate identical replacements in diagnostics.
   // FIXME: Find an efficient way to deduplicate on diagnostics level.
-  llvm::DenseMap>
+  llvm::DenseMap>
   DiagReplacements;
 
-  auto AddToGroup = [&](const tooling::Replacement , bool FromDiag) {
+  auto AddToGroup = [&](const tooling::Replacement ,
+const tooling::TranslationUnitDiagnostics *FromTU) {
 // Use the file manager to deduplicate paths. FileEntries are
 // automatically canonicalized.
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
-  if (FromDiag) {
+  if (FromTU) {
 auto  = DiagReplacements[*Entry];
-if (!Replaces.insert(R).second)
+if (Replaces.find(R) == Replaces.end())
+  Replaces.emplace(R, FromTU);
+else if (Replaces.at(R) != FromTU)
   return;
   }
   GroupedReplacements[*Entry].push_back(R);
@@ -166,14 +171,14 @@
 
   for (const auto  : TUs)
 for (const tooling::Replacement  : TU.Replacements)
-  AddToGroup(R, false);
+  AddToGroup(R, nullptr);
 
   for (const auto  : TUDs)
 for (const auto  : TU.Diagnostics)
   if (const 

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Test cases will follow, just time constrained for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76054



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

[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, gribozavr2, AlexanderLanin.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
njames93 added a comment.

Test cases will follow, just time constrained for now.


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 export-fixes to 
yaml adds extra newlines and breaks offsets. 



Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76054

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


Index: 
clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -145,16 +145,21 @@
 
   // Deduplicate identical replacements in diagnostics.
   // FIXME: Find an efficient way to deduplicate on diagnostics level.
-  llvm::DenseMap>
+  llvm::DenseMap>
   DiagReplacements;
 
-  auto AddToGroup = [&](const tooling::Replacement , bool FromDiag) {
+  auto AddToGroup = [&](const tooling::Replacement ,
+const tooling::TranslationUnitDiagnostics *FromTU) {
 // Use the file manager to deduplicate paths. FileEntries are
 // automatically canonicalized.
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
-  if (FromDiag) {
+  if (FromTU) {
 auto  = DiagReplacements[*Entry];
-if (!Replaces.insert(R).second)
+if (Replaces.find(R) == Replaces.end())
+  Replaces.emplace(R, FromTU);
+else if (Replaces.at(R) != FromTU)
   return;
   }
   GroupedReplacements[*Entry].push_back(R);
@@ -166,14 +171,14 @@
 
   for (const auto  : TUs)
 for (const tooling::Replacement  : TU.Replacements)
-  AddToGroup(R, false);
+  AddToGroup(R, nullptr);
 
   for (const auto  : TUDs)
 for (const auto  : TU.Diagnostics)
   if (const auto *ChoosenFix = tooling::selectFirstFix(D)) {
 for (const auto  : *ChoosenFix)
   for (const tooling::Replacement  : Fix.second)
-AddToGroup(R, true);
+AddToGroup(R, );
   }
 
   // Sort replacements per file to keep consistent behavior when


Index: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
===
--- clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
+++ clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
@@ -145,16 +145,21 @@
 
   // Deduplicate identical replacements in diagnostics.
   // FIXME: Find an efficient way to deduplicate on diagnostics level.
-  llvm::DenseMap>
+  llvm::DenseMap>
   DiagReplacements;
 
-  auto AddToGroup = [&](const tooling::Replacement , bool FromDiag) {
+  auto AddToGroup = [&](const tooling::Replacement ,
+const tooling::TranslationUnitDiagnostics *FromTU) {
 // Use the file manager to deduplicate paths. FileEntries are
 // automatically canonicalized.
 if (auto Entry = SM.getFileManager().getFile(R.getFilePath())) {
-  if (FromDiag) {
+  if (FromTU) {
 auto  = DiagReplacements[*Entry];
-if (!Replaces.insert(R).second)
+if (Replaces.find(R) == Replaces.end())
+  Replaces.emplace(R, FromTU);
+else if (Replaces.at(R) != FromTU)
   return;
   }
   GroupedReplacements[*Entry].push_back(R);
@@ -166,14 +171,14 @@
 
   for (const auto  : TUs)
 for (const tooling::Replacement  : TU.Replacements)
-  AddToGroup(R, false);
+  AddToGroup(R, nullptr);
 
   for (const auto  : TUDs)
 for (const auto  : TU.Diagnostics)
   if (const auto *ChoosenFix = tooling::selectFirstFix(D)) {
 for (const auto  : *ChoosenFix)
   for (const tooling::Replacement  : Fix.second)
-AddToGroup(R, true);
+AddToGroup(R, );
   }
 
   // Sort replacements per file to keep consistent behavior when
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits