[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<const FileEntry *, std::set<tooling::Replacement>>
+  llvm::DenseMap<const FileEntry *,
+                 std::map<tooling::Replacement,
----------------
Please update the comments to explain the new structure.


================
Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:154
+  auto AddToGroup = [&](const tooling::Replacement &R,
+                        const tooling::TranslationUnitDiagnostics *FromTU) {
     // Use the file manager to deduplicate paths. FileEntries are
----------------
nit: I'd rename to `SourceTU`. Before, `FromDiag` was used as a proposition, as in "from diag?". Here, it's an (optional) value, so I think it would be better to name it that way.


================
Comment at: clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:160
         auto &Replaces = DiagReplacements[*Entry];
-        if (!Replaces.insert(R).second)
+        if (Replaces.find(R) == Replaces.end())
+          Replaces.emplace(R, FromTU);
----------------
This results in a double lookup. Instead, maybe:
```
auto It = Replaces.find(R);
if (It == Replaces.end())
  Replaces.emplace(R, FromTU);
else if (*It != FromTU)
  // This replacement is a duplicate of one suggested by another TU.
  return;
```


================
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
----------------
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