ioeric added inline comments.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:188
-bool mergeAndDeduplicate(const TUReplacements &TUs,
- FileToReplacementsMap &GroupedReplacements,
- clang::SourceManager &SM) {
-
- // Group all replacements by target file.
+static llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
+groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
----------------
Add a brief comment describe what this function does, specifically about `TUs`
and `TUDs`.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:190
+groupReplacements(const TUReplacements &TUs, const TUDiagnostics &TUDs,
+ FileToChangesMap &FileChanges, clang::SourceManager &SM) {
std::set<StringRef> Warned;
----------------
nit: `SM` can be `const`. And put `SM` (input arg) before `FileChanges` (output
arg).
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:213-221
// Use the file manager to deduplicate paths. FileEntries are
// automatically canonicalized.
- if (const FileEntry *Entry =
SM.getFileManager().getFile(R.getFilePath())) {
+ if (const FileEntry *Entry =
+ SM.getFileManager().getFile(R.getFilePath())) {
GroupedReplacements[Entry].push_back(R);
} else if (Warned.insert(R.getFilePath()).second) {
+ errs() << "Described file '" << R.getFilePath()
----------------
This code block is the same as the one in the above loop. Consider pulling it
into a lambda.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:276-279
+ if (R.getLength() == 0)
+ addInsertion(R);
+ else
+ addReplacement(R);
----------------
`AtomicChange::replace` also handles insertions, so I think there is no need
for the distinction here.
(See my comment in where `ReplacementsToAtomicChanges` is used; I think the
class might not be needed.)
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:290
+ std::vector<tooling::AtomicChange> Changes;
+ for (const auto &R : AllChanges.getReplacements()) {
+ tooling::AtomicChange Change(Entry->getName(), Entry->getName());
----------------
I might be missing something, but why do we need to pull replacements from the
result change into a set of changes?
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:317
+ if (llvm::Error Error =
+ AllChanges.insert(*SM, BeginLoc.getLocWithOffset(R.getOffset()),
+ R.getReplacementText())) {
----------------
By default, this inserts R after the existing insertion in the same location
(if there is any). But it's impossible for apply-all-replacements to know in
which order they should be applied, so I would suggest treating this as
conflict. Sorry that I might have confused you by asking to add a test case
where two identical insertions are both applied (because they are order
*independent*), but `AtomicChange::replace` also supports that and would report
error when two insertions are order *dependent*.
`AtomicChange::replace` and `AtomicChange::insert` have almost the same
semantic except that you could specify the expected order of insertions on the
same location with `insert`, but in our case, we wouldn't know which order is
desired.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:353
+ const FileEntry *Entry = FileAndReplacements.first;
+ ReplacementsToAtomicChanges DeduplicatedChanges(SM, Entry);
+ for (const auto &R : FileAndReplacements.second)
----------------
Sorry that I didn't make myself clear... what you had in the previous revision
was correct (for the current use case of apply-all-replacements) i.e. store all
replacements in one `AtomicChange`, which allows you to detect conflicts on the
fly. So the code here can be simplified as:
```
...
Entry = ...;
AtomicChange FileChange;
for (const auto &R : FileAndReplacements.second) {
auto Err = FileChange.replace( <args from R> );
if (Err)
reportConflict(Entry, std::move(Err)); // reportConflict as we go
}
FileChanges.emplace(Entry, {FileChange});
...
```
I think with this set up, you wouldn't need `ReplacementsToAtomicChanges`,
`ConflictError` and `reportConflicts`.
================
Comment at: test/clang-apply-replacements/Inputs/basic/file2.yaml:1
---
MainSourceFile: source2.cpp
----------------
Could you please add two test cases for insertions: 1) identical insertions
(e.g. "AB" and "AB") at the same location are applied sucessfully and 2)
order-dependent insertions (e.g. "AB" and "BA") are detected.
(It might make sense to do this in a different file)
================
Comment at: test/clang-apply-replacements/Inputs/conflict/expected.txt:8
+ Replace 9:5-9:11 with "elem"
The following changes conflict:
Remove 12:3-12:14
----------------
How could one replacement (`Remove 12:3-12:14`) conflict with itself?
https://reviews.llvm.org/D43764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
