omtcyf0 added a subscriber: omtcyf0. omtcyf0 added a comment. Hi @vmiklos!
Thank you very much for contributing to clang-rename. The patch looks nice, but it conflicts with my understanding of the view on what the tool should do. Generally, I do not support the idea of adding an option to perform multiple renamings at once. Here are my reasons: - I think the tool should mainly target editors. Most users won't use command line interface IMHO. Thus, performing one renaming action at once seems more than logical to me, because you don't do multiple in the editor. - There's a patch I wish to put for review soon, which is about checking -new-name - whether it is a valid identifier or not. This will be the first step towards teaching the tool to understand whether the change will break the codebase or not (i.e. there will be name conflict, invalid identifier or something else). So, we'll have to check each "new-name" for being valid, check each "old-name" for being valid, check whether we have the same number of "new-name"s and "old-name"s (or offsets) [which you have in your patch], then for each pair check whether the renaming action would be OK. - With this patch we're bringing tokenization of these names into ClangRename.cpp, which doesn't seem cool to me. I think the main file is supposed to be tool-logic only, not sure if putting much logic-unrelated code is good there. To sum up, my main points are: - I think we should try to make a good interface, which can be easily reused by the editors instead of trying to target CLI. You can try very simple vim integration, see latest http://reviews.llvm.org/D22087. - I believe the tool shouldn't try to do literally everything and get features, which require much hardcoding and cornercases handling. Doing only one exact thing and doing it very good seems reasonable to me. Anyway, if everyone is happy about moving in this direction aswell, I'm fine too, but these are just my thoughts. If you want to chat, feel free to drop me an email :) http://reviews.llvm.org/D21814 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
