ioeric added a comment.
Thanks for the cleanup! This looks really nice!
================
Comment at:
clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h:45
+/// \brief Map mapping file name to AtomicChange targeting that file.
+typedef llvm::DenseMap<const clang::FileEntry *, tooling::AtomicChange>
+ FileToChangeMap;
----------------
I would expect this to be a map from files to a group of AtomicChanges (e.g.
`std::vector<AtomicChange>`). In general, a single AtomicChange contains
changes around a single code location which are likely to conflict with each
other, and either all changes or no change is applied. A file usually
corresponds to a set of atomic changes.
Intuitively, I think clang-apply-replacements should simple gather a set of
atomic changes for each file and let `applyAtomicChanges` handle the conflicts,
but its current behavior is to skip conflicting replacements and keep applying
other replacements. This is not ideal, but I guess I understand where this came
from, and we might not be able to fix this in this patch since most tools
produce replacements instead of AtomicChange.
I would still suggest make this a map something like `llvm::DenseMap<const
clang::FileEntry *, std::vector<tooling::AtomicChange>>`. To preserve the
current behavior (i.e. skip conflicts), when you group all replacements for a
single file, you would still put all replacements into a single AtomicChange,
but when you actually put the change into the map, you put it as a vector of a
single change.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:207
+ llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement, LessNoPath>>
+ GroupedReplacements;
+
----------------
I don't think we need to do the deduplication here anymore. AtomicChange
handles duplicates for you. I think all you need to do here is to group
replacements by files and later convert replacements to atomic changes.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:248
+ const auto BeginLoc = SM.getLocForStartOfFile(FID);
+ tooling::AtomicChange Change(Entry->getName(), Entry->getName());
+ for (const auto &R : FileAndReplacements.second)
----------------
(Related to my comment on `FileToChangeMap`)
We should add FIXME here explaining why we are putting all replacements for a
single file into one AtomicChange (e.g. we want to be able to detect and report
each individual conflict) instead of a set of changes.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:259
+ }
+ Changes.insert(std::make_pair(Entry, std::move(Change)));
}
----------------
(Also related to my comment on FileToChangeMap)
This would be something like
```
Changes.insert(std::make_pair(Entry, {Change}));
```
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:279
+ if (!NewCode) {
+ errs() << "Failed to apply fixes on " << File << "\n";
+ return false;
----------------
You should handle the error in `llvm::Expected`. You could convert it to string
and add to the error message with `llvm::toString(NewCode.takeError())`. It
would be nice if we could have a test case for such cases.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43764
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
