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

Kirill Bobyrev via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 29 02:04:20 PDT 2016


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





More information about the cfe-commits mailing list