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