[PATCH] D76054: [clang-apply-replacements] No longer deduplucates replacements from the same TU
Yitzhak Mandelbaum via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 23 06:31:04 PDT 2020
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
More information about the cfe-commits
mailing list