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