[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/include/clang-apply-replacements/Tooling/ApplyReplacements.h:75
+/// \brief Deduplicate, check for conflicts, and convert all Replacements stored
+/// in \c TUs to AtomicChange. If conflicts occur, no Replacements are applied.
 ///
----------------
ioeric wrote:
> ` If conflicts occur, no Replacements are applied.` This doesn't seem to be accurate; non-conflicting replacements are still added.
You are perfectly right the description does not match the behavior.

I think I will update this part to //"Deduplicate, check for conflicts, and convert all Replacements stored in TUs to a single AtomicChange per file. Conflicting replacement are skipped. However, the order of converting Replacements extracted from TUs and TUDs to AtomicChange is undefined."//

And amend `FileChanges` with //"Only non conflicting replacements are kept into FileChanges."//


================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:179
+  // To keep current clang-apply-replacement behavior, report conflicting
+  // replacements skip file containing conflicts, all replacements are stored
+  // into 1 big AtomicChange.
----------------
ioeric wrote:
> I think we are only skipping conflicts; non-conflicting replacements are still added even if the file contains other conflicts?
> 
> This doesn't seem to have caused any problem because the current caller simply drops all changes if conflict is detected in any file, but we should still make it clear what the behavior of `mergeAndDeduplicate` is (in the API doc) e.g.  what would `FileChanges` contain if there is conflict in some but not all files?
I will update this comment to:
//"To keep current clang-apply-replacement behavior, report conflicting replacements on corresponding file, all replacements are stored into 1 big AtomicChange."//


https://reviews.llvm.org/D43764