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