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

[Kirill Bobyrev via cfe-commits]

omtcyfz added a comment.

In https://reviews.llvm.org/D21814#500481, @vmiklos wrote:

> > 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. :)


Yes, but I don't think there are many locations where `clang-format` would be triggered. I believe I ran clang-format over most parts few times.

> 

> 

> > 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.


Cool.

> 

> 

> > 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).

> 

> 

> 


Removed @; otherwise Eugene becomes subscribed :)

> 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`.


Aha, I see. Yes, for `vector<vector>>` it seems reasonable. Sorry, didn't that it's `vector<vector>>`.

> 

> 

> > 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. :)


//looking into your honest eyes//

> 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.


As for help message, look at `clang-tidy`. Is there a need in `helpMain`?


https://reviews.llvm.org/D21814