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

[Jeremy Demeule via Phabricator via cfe-commits]

jdemeule added inline comments.


================
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()
----------------
ioeric wrote:
> This code block is the same as the one in the above loop. Consider pulling it into a lambda.
Yes, for sure. I was not confident about this part.


================
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());
----------------
ioeric wrote:
> I might be missing something, but why do we need to pull replacements from the result change into a set of changes? 
I interpret you suggestion of set of `AtomicChange` a little bit too much.


================
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)
----------------
ioeric wrote:
> 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`.
I think I have over-interpreting your previous comment :-)
However, I am still confused about error reporting.
Currently, clang-apply-replacements reports conflicting replacement per file and per conflict.
For example:
```
There are conflicting changes to XXX:
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Replace 8:8-8:33 with "BBB"
  Remove 8:10-8:15
  Insert at 8:14 CCC
```

With this patch, conflict are reported by pair (first replacement/conflicted one) and I am able to print the corresponding file only once (thanks to the defered reporting).
```
There are conflicting changes to XXX:
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Replace 8:8-8:33 with "BBB"
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Remove 8:10-8:15
The following changes conflict:
  Replace 8:8-8:33 with "AAA"
  Insert at 8:14 CCC
```

I prefer the way you suggest to report conflict but we will loose or print conflicting file at each conflict detected.
I even more prefer to use `llvm::toString(std::move(Err))` but this will fully change the reporting and will also be reported by pair.
```
The new replacement overlaps with an existing replacement.
New replacement: XXX: 106:+26:"AAA"
Existing replacement: XXX: 106:+26:"BBB"
The new replacement overlaps with an existing replacement.
New replacement: XXX: 106:+26:"AAA"
Existing replacement: XXX: 112:+0:"CCC"
The new replacement overlaps with an existing replacement.
New replacement: XXX: 106:+26:"AAA"
Existing replacement: XXX: 108:+12:""
```


================
Comment at: test/clang-apply-replacements/Inputs/basic/file2.yaml:1
 ---
 MainSourceFile:     source2.cpp
----------------
ioeric wrote:
> 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)
I will.


================
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
----------------
ioeric wrote:
> How could one replacement (`Remove 12:3-12:14`) conflict with itself?
It is not the case, they are reported by pair now.


https://reviews.llvm.org/D43764