[PATCH] D21814: clang-rename: split existing options into two new subcommands

[Miklos Vajna via cfe-commits]

vmiklos added a subscriber: Eugene.Zelenko.
vmiklos added a comment.

> 1. Run `clang-format` or something, 80 char width limit is broken in `tool/ClangRename.cpp` dozen of times.


Done. I was afraid doing that, due to the changes not related to my patch, but
the result doesn't seem to be too bad. I guess in a later patch it would be
great to run clang-format on the whole clang-rename code, then all contributors
could just run clang-format before committing in case LLVM coding style is not
in our muscle memory. :)

> 2. Only do `outs() << "abcd\n" << "efgh\n"` if you have something in between, which can not be predefined. I.e. if you have ``` /// \brief Top level help. static int helpMain(int argc, const char *argv[]) { errs() << "Usage: clang-rename {rename-at|rename-all} [OPTION]...\n\n" << "A tool to rename symbols in C/C++ code.\n\n" << "Subcommands:\n" << "  rename-at:  Perform rename off of a location in a file. (This is the default.)\n" << "  rename-all: Perform rename of all symbols matching one or more fully qualified names.\n"; return 0; } ``` Just make it a const string, isn't it better?


Done. I copied llvm-cov, but I have no problems changing it.

> 3. `tooling::RefactoringTool` takes care of printing version IIUC, no need to do that in a custom way (we don't have custom version in `clang-rename` head at the moment, that was something @Eugene.Zelenko sent recently).


Indeed, removed.

> 4. `clang-rename/RenamingAction.h` -> `USRList` -> `USRs` or the other way around everywhere. So far single naming convention feels right to me (I personally prefer `*s` over `*List`). LLVM Coding Style <http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly> does, too, from what I understand. Unless it's `*Set`, which is pretty much different thing.


I've changed NewNameList and PrevNameList.

USRList refers to a list of lists, i.e. doing one oldname->newname rename may
have to deal with multiple USRs, and when doing multiple oldname->newname
renames, you need to deal with a list of list of USRs. I used the List suffix
in this "list of list" case. I have no problem renaming
`std::vector<std::vector<std::string>> USRList` to USRs, but then we need a new
name for `std::vector<std::string> USRs`.

> 5. Do we really need to dispatch these functions this way? with

> 

>   ``` enum RenameSubcommand { RenameAt, RenameAll }; ``` And many other strange things. Just pass `bool isMultipleRename` or `isRenameAll` to `main` routine instead of creating `enum`. We only have two such options here, right? I pray we won't have more.


I promise I don't plan to suggest more. :) Changed the enum to a bool.

> 6. Move `int main(int argc, const char **argv)` upwards, so that it's above dispatched routines. Otherwise when one wants to see what's going on, he sees subroutine first without understanding where it comes from. Other way around feels more intuitive to me.


Done.


https://reviews.llvm.org/D21814