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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
