djasper added inline comments. ================ Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:137 @@ +136,3 @@ + std::end(NewWrittenInitializersOrder), ByFieldNewPosition); + assert(OldWrittenInitializersOrder.size() == + NewWrittenInitializersOrder.size()); ---------------- djasper wrote: > This assert seems pretty useless. Actually no, this is fine.
================ Comment at: clang-reorder-fields/ReorderFieldsAction.h:31 @@ +30,3 @@ + +public: + ReorderFieldsAction( ---------------- ioeric wrote: > 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. Ok.. Then nevermind and sorry for the noise.. Defining a typedef as Eric suggests probably makes this a bit easier to read. 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
