[PATCH] D23279: clang-reorder-fields

[Alexander Shaposhnikov via cfe-commits]

alexshap added a subscriber: ioeric.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.h:31
@@ +30,3 @@
+
+public:
+  ReorderFieldsAction(
----------------
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


Repository:
  rL LLVM

https://reviews.llvm.org/D23279