jdemeule added a comment.
Thanks for your feedback, they are very precious to me!
================
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;
----------------
ioeric wrote:
> 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.
I got your point. I will update the patch this way.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:207
+ llvm::DenseMap<const FileEntry *, std::set<tooling::Replacement, LessNoPath>>
+ GroupedReplacements;
+
----------------
ioeric wrote:
> 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.
I think I wrongly use AtomicChange somewhere because it doesn't deduplicate
same replacement automatically.
For exemple, in the test suite, basic test defines 2 time the same replacement
(adding 'override ' at offset 148) and I do not manage to avoid AtomicChange to
add 'override override '. This is why I have kept the deduplicate step.
================
Comment at: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp:279
+ if (!NewCode) {
+ errs() << "Failed to apply fixes on " << File << "\n";
+ return false;
----------------
ioeric wrote:
> 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.
I will use `llvm::Expected` as you suggest.
Do you have some ideas to made a test failed at this level?
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
