ioeric added a comment.
This looks great! Just a few nits left.
================
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.
///
----------------
` If conflicts occur, no Replacements are applied.` This doesn't seem to be
accurate; non-conflicting replacements are still added.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:173
- // Group all replacements by target file.
- std::set<StringRef> Warned;
- for (const auto &TU : TUs) {
- for (const tooling::Replacement &R : TU.Replacements) {
- // Use the file manager to deduplicate paths. FileEntries are
- // automatically canonicalized.
- 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()
- << "' doesn't exist. Ignoring...\n";
- }
- }
- }
+ llvm::DenseMap<const FileEntry *, std::vector<tooling::Replacement>>
+ GroupedReplacements = groupReplacements(TUs, TUDs, SM);
----------------
nit: I'd probably use `auto GroupedReplacements = ...;`.
================
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.
----------------
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?
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:218
+ DiagnosticsEngine &Diagnostics) {
+ FileManager Files((FileSystemOptions()));
----------------
nit: redundant empty line.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:227
- return true;
+ StringRef Code = Buffer.get()->getBuffer();
+ return tooling::applyAtomicChanges(File, Code, Changes, Spec);
----------------
nit: consider inline this to save one variable.
https://reviews.llvm.org/D43764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
