[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 accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

lgtm



================
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.
 ///
----------------
jdemeule wrote:
> 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."//
> a single AtomicChange per file
As `FileChanges` are a `FileToChangesMap`, it's already clear that it's a set of changes per file. We don't want to say "single AtomicChange" here because this is just an implementation details. 


================
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.
----------------
jdemeule wrote:
> 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."//
I would drop the "To keep the current behavior" part because it's only relevant in code review. Future readers wouldn't know what the "current" behavior is. 


https://reviews.llvm.org/D43764