[PATCH] D23279: clang-reorder-fields

[Daniel Jasper via cfe-commits]

djasper added inline comments.

================
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) {
----------------
alexshap wrote:
> 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.
If you don't reorder a partial initializer then I think it is quite likely that the resulting code doesn't compile or, worse, compiles but has incorrect behavior. I don't think this is any better or worse than not reordering fields across different access specifiers.

Also, I don't understand what the problem is. Just create separate local variable for Replacements and only copy to the class member if everything is successful. Or clear Replacements if something goes wrong. Am I missing something?


Repository:
  rL LLVM

https://reviews.llvm.org/D23279