ioeric added inline comments. ================ Comment at: clang-reorder-fields/ReorderFieldsAction.h:31 @@ +30,3 @@ + +public: + ReorderFieldsAction( ---------------- alexshap wrote: > alexshap wrote: > > djasper wrote: > > > I don't why clang-rename does what it does at the moment. My main point > > > is that we have already implemented splitting up replacements by file in > > > groupByReplacementsByFile. No need to reimplement that here or make the > > > interface of this function more complex than it needs to be. > > sorry, i am not sure i got ur comment - need to think - maybe i am too > > sleepy - > > the issue is not with clang-rename, but with class RefactoringTool > > and more precisely with the method the method getReplacements. > pls, take a look at my previous comments (especially regarding the diff > https://reviews.llvm.org/D21748?id=64016.) > > meanwhile: > http://clang.llvm.org/doxygen/Replacement_8h_source.html > > **/// \brief Adds a new replacement \p R to the current set of replacements. > 160 /// \p R must have the same file path as all existing replacements. ** > 161 /// Returns true if the replacement is successfully inserted; > otherwise, > 162 /// it returns an llvm::Error, i.e. there is a conflict between R and > the > 163 /// existing replacements or R's file path is different from the > filepath of > 164 /// existing replacements. Callers must explicitly check the Error > returned. > 165 /// This prevents users from adding order-dependent replacements. To > control > 166 /// the order in which order-dependent replacements are applied, use > 167 /// merge({R}) with R referring to the changed code after applying all > 168 /// existing replacements. > 169 /// Replacements with offset UINT_MAX are special - we do not detect > conflicts > 170 /// for such replacements since users may add them intentionally as a > special > 171 /// category of replacements. > 172 llvm::Error add(const Replacement &R); > > at this point it seems like groupReplacementsByFile was made redundant by > https://reviews.llvm.org/D21748 > > @ioeric, could u comment on this ? I apologize in advance - i might be > missing smth / might be to sleepy. Will take a look at this again tomorrow > > P.S. just in case - ReorderingFieldsAction can change different files > (including headers etc) - so i simply can not use a single Replacements object We don't have an implementation for grouping replacements by file yet (for class implementation of `tooling::Replacements`). The previous `groupReplacementsByFile` indeed should have been deprecated. At this point, tools which carry replacements for multiple files use a map from file names to `tooling::Replacements` (usually called `FileToReplacements`) like what you do here.
Repository: rL LLVM https://reviews.llvm.org/D23279 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
