omtcyfz added a comment.

The patch looks fine to me (though I'm not sure if there are no new tests; if 
they are interface changes should be applied).

If everyone seems to be in favor of such changes, I'm OK with it, but in 
general I think it makes things more complicated and I'm not sure if it's 
necessary at the moment; I expressed my ideas about it in comments to the other 
patch. But if that's what the common use-case is... So, //TL;DR// I personally 
don't see why one would want to rename multiple things at once while we still 
can't rename a single symbol correctly in too many cases...

P.S. it seems logical to me to support `-offset` option in `-rename-all`, too. 
And introducing `-rename-all` without actually supporting multiple renaming 
actions "at once" seems weird to me, too.


================
Comment at: clang-rename/tool/ClangRename.cpp:226
@@ +225,3 @@
+  if (argc > 1) {
+    typedef int (*MainFunction)(int, const char *[]);
+    MainFunction Func = StringSwitch<MainFunction>(argv[1])
----------------
use `std::function` here?


https://reviews.llvm.org/D21814



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to