[PATCH] D43764: [clang-apply-replacements] Convert tooling::Replacements to tooling::AtomicChange for conflict resolving of changes, code cleanup, and code formatting.

[Eric Liu via Phabricator via cfe-commits]

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