omtcyfz added a subscriber: Eugene.Zelenko.
omtcyfz added a comment.
1. Run `clang-format` or something, 80 char width limit is broken in
`tool/ClangRename.cpp` dozen of 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?
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).
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.
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.
6. Move `int main(int argc, const char **argv)` upwards, so that it's above
dispatching 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.
Feel free do disagree with my points, I may be terribly wrong :)
https://reviews.llvm.org/D21814
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
