[PATCH] D23279: clang-reorder-fields

[Daniel Jasper via cfe-commits]

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