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