[PATCH] D23279: clang-reorder-fields

[Alexander Shaposhnikov via cfe-commits]

alexshap added inline comments.

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:83
@@ +82,3 @@
+static void
+addReplacement(SourceRange Old, SourceRange New, const ASTContext &Context,
+               std::map<std::string, tooling::Replacements> &Replacements) {
----------------
khm, it Replaces Old by New, not vice versa - so it's not a swap.
(and semantically we are not doing swaps either, but we extract the source code from some places and insert it into some other places) 

================
Comment at: clang-reorder-fields/ReorderFieldsAction.cpp:178
@@ +177,3 @@
+    const InitListExpr *InitListEx, ArrayRef<unsigned> NewFieldsOrder,
+    const ASTContext &Context,
+    std::map<std::string, tooling::Replacements> &Replacements) {
----------------
yeah, i am aware of it. In general - yes, you are right.
But to me it seems that for now it would be better to leave it as is (don't drop all the other replacements because of this one case - now we write a message about this issue to llvm::errs()).
The other options don't look very good to me (or at least better) so decided not to make things complicated (for now). Probably i need to provide more context / explanations. Right now in my code there are two places where we return true/false to signal about an issue and also write a message to llvm::errs(). The first one is reorderFieldsInDefinition and the second one is  reorderFieldsInInitListExpr. The first seems to be critical (if we are not able to edit the definition doing anything else doesn't make sense - now it works this way - so we should be good), the second (reorderFieldsInInitListExpr) doesn't seem to be so critical (partial initialization of an aggregate), so i would not like to drop all the other replacements because of it. Okay, that was one part of the story. The other part of the story: 
A.
   void HandleTranslationUnit(ASTContext &Context) override
 - it doesn't return anything (so i can not propagate the return code) (to be honest i can, but not sure if those changes are really worth doing (taking into account the context i described above))
B.
   http://clang.llvm.org/doxygen/CompilerInstance_8cpp_source.html#l00871
   line 871  Act.Execute();     (return value is ignored)
-- but this seems to be beyond the scope of my diff, and as i said above - doing smth more complicated only for partial initialization (which is now not supported by this tool anyway) - not sure if it's worth doing right now.


Repository:
  rL LLVM

https://reviews.llvm.org/D23279