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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
