Page MenuHomePhabricator

clang-rename: split existing options into two new subcommands
ClosedPublic

Authored by vmiklos on Jun 28 2016, 1:21 PM.

Details

Summary
  • rename-at is meant to be integrated with editors and works mainly off of a location in a file, and this is the default
  • rename-all is optimized for one or more oldname->newname renames, and works with clang-apply-replacements

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I think we really want 2 tools:
a) one that is optimized for oldname->newname renames, and supports the multi-TU case really well
b) one that is meant to be integrated with editors and works mainly off of a location in a file

I'm a bit torn whether putting those 2 into the same executable is a good idea. Looping in Benjamin for additional ideas.

bkramer edited edge metadata.Jul 7 2016, 5:15 AM

I think we really want 2 tools:
a) one that is optimized for oldname->newname renames, and supports the multi-TU case really well
b) one that is meant to be integrated with editors and works mainly off of a location in a file

I'm a bit torn whether putting those 2 into the same executable is a good idea. Looping in Benjamin for additional ideas.

I'm fine with both things living in the same binary. The location thing is just a different way of specifying the symbol name. I fail to see how that's related to this review though.

klimek added a comment.Jul 7 2016, 6:04 AM

I think we really want 2 tools:
a) one that is optimized for oldname->newname renames, and supports the multi-TU case really well
b) one that is meant to be integrated with editors and works mainly off of a location in a file

I'm a bit torn whether putting those 2 into the same executable is a good idea. Looping in Benjamin for additional ideas.

I'm fine with both things living in the same binary. The location thing is just a different way of specifying the symbol name. I fail to see how that's related to this review though.

Well, it's about how much features we want to pack into the same binary; once we basically have multiple tools in one tool, adding more different use cases makes sense, I think.

omtcyf0 added a subscriber: omtcyf0.EditedJul 7 2016, 11:19 AM

Hi @vmiklos!

Thank you very much for contributing to clang-rename.

The patch looks nice, but it conflicts with my understanding of the view on what the tool should do.

Generally, I do not support the idea of adding an option to perform multiple renamings at once. Here are my reasons:

  • I think the tool should mainly target editors. Most users won't use command line interface IMHO. Thus, performing one renaming action at once seems more than logical to me, because you don't do multiple (at once) in the editor.
  • There's a patch I wish to put for review soon, which is about checking -new-name - whether it is a valid identifier or not. This will be the first step towards teaching the tool to understand whether the change will break the codebase or not (i.e. there will be name conflict, invalid identifier or something else). So, we'll have to check each "new-name" for being valid, check each "old-name" for being valid, check whether we have the same number of "new-name"s and "old-name"s (or offsets) [which you have in your patch], then for each pair check whether the renaming action would be OK.
  • With this patch we're bringing tokenization of these names into ClangRename.cpp, which doesn't seem cool to me. I think the main file is supposed to be tool-logic only, not sure if putting much logic-unrelated code is good there.

To sum up, my main points are:

  • I think we should try to make a good interface, which can be easily reused by the editors instead of trying to target CLI. You can try very simple vim integration, see latest http://reviews.llvm.org/D22087.
  • I believe the tool shouldn't try to do literally everything and get features, which require much hardcoding and cornercases handling. Doing only one exact thing and doing it very good seems reasonable to me.

Anyway, if everyone is happy about moving in this direction aswell, I'm fine too, but these are just my thoughts.

If you want to chat, feel free to drop me an email :)

Kirill: OK, so you're in the camp marked as b) by Manuel. Sure, the vim integration is nice (I'm also a vim user), now that you mentioned it, I need to go and try it myself. ;-) Given the above patch, probably it's obvious that I'm more in camp a). I don't insist on having that in a tool named clang-rename, though.

Hmm, so here is the summary of the comments so far, as far as I understand:

  • Manuel: it's OK to handle multiple renames with a single command-line invocation, but not sure if clang-rename should do that, or if it should be a separate tool
  • Benjamin: no problem with clang-rename having this feature
  • Kirill: clang-rename should definitely not have this feature.

What's the consensus, should I rework this patch, so that it doesn't touch tool/ClangRename.cpp, but adds a new tool (named e.g. clang-multi-rename, still linking to clangRename)? I can do that, but it would be good to hear first if it is worth the effort, or there would be still "the patch is correct, but it's not the way to go" style comments. :-)

Thanks.

omtcyf0 added a comment.EditedJul 7 2016, 1:54 PM

Miklos, thanks for the feedback!

Hm, I'm not sure about a) and b) camps here. I think we can have both. It may be that I haven't looked too much into the code or I am missing something, but so far both integration and cross-TU analysis seem OK together in one tool as for me.

So far, integration (in it's very very simple way) is very straightforward: we just have a binary and we call it via passing two parameters (offset and new-name). This doesn't require neither changes in the tool as it is nor any difficult tweaks.

As for cross-TU stuff, why can't it be here, too? We'll still need it at some point.

Creating clang-multi-rename doesn't seem right at all - there are already too many tools which some users don't understand. If the command line interface with this feature is a useful thing to have, then we should definitely just add it here. One issue I see is that I think it won't be useful to most users, but a) I can be wrong b) It's not the main one :)

Probably my main point is: let's not bring too much features into the interface of the yet non-correctly working tool. It feels right to me to ensure first that the tool actually works correctly. Because it does not at the moment (see http://reviews.llvm.org/D22102 XFAIL test for example).

After we do that, we will have the feedback from the community, because they will try the first version, which actually works. We might get at least few people just trying it at least and saying whether for example it's not usable at all in the editor and that they would like an extended CLI. I'll be all for the changes similar to these proposed by you then!

In my opinion we should (right now) only support a single use scenario: clang-rename -offset=[OFFSET] -new-name=[NAME]. Not only because we would keep things easy (which I'd honestly be very happy about, but, I know, people hate me for that :D), but because when we get our first users, chances of them finding some configuration of parameters which breaks than simples single scenario is very high, not talking about the multiple scenarios you're proposing. Why would we need many ways to interact with an incorrect tool? We should probably have a single way to interact with a completely correct tool.

At the end I think we can both be happy if we separate the CLI and the renaming interface completely. The editors can interact with a C++ Library then and clang-rename would become a (probably) sophisticated command line interface to that library, only dealing with user-library interaction thingie. We can't have both at the moment, of that I am certain.

My proposal is to:

  • Improve Vim interface just a little, I think I can do that tomorrow.
  • Write a letter to the cfe-dev, explaining that we want clang-rename to become useful and that we want their feedback. I'll send it tomorrow, right after I'll be done with the Vim interface slight improvements. We'll probably get more interesting ideas and, most importantly, at least few first users, who will help us to understand what they need. Discussions on reviews get lost and forgotten, cfe-dev is always there and living.
  • Get a proper test set for clang-rename. Really. Because now it's not tested at all. Fill in bugs (hopefully the guys who will try it at least will help), get suggestions, get opinions. Probably make a list/plan of them.
  • Address the issues and then think of the additional stuff we can add (like separating the interface and routine and all that).

As I'm going to invest very much time into the project I hope we can do that!

I'll be very happy to hear more opinions from the others both here and in the cfe-dev list after I send the message!

@klimek, @bkramer, what do you think of this?

klimek added a comment.Jul 7 2016, 2:24 PM

Kirill, I think it's important to note that Miklos is probably a user, otherwise I'd guess he wouldn't add those features.
Generally, we also internally have a (large scale) simplified rename tool that allows renaming multiple things in a code base at once; it is definitely a useful tool to have.
I believe that Benjamin expresses the opinion of multiple people in the community (usually including Chandler) who want fewer binaries.
Perhaps we can make this better by doing the following compromise:
Instead of just having tons of parameters where it's hard to say which parameter does what, have 2 different commands that you can call, each with their own set of flags:
clang-rename rename-at -offset ... -newname ...
clang-rename rename-all -oldname ... -newname ... -oldname ... -new-name ... (or similar)

Manuel,

I think it's important to note that Miklos is probably a user, otherwise I'd guess he wouldn't add those features.

Sure. But there are different kinds of users :) IIRC there's a discussion in cfe-def called "Clang should natively support Fortran" or something. Not saying it's bad, but needs of different users may seem strange to the others.

Generally, we also internally have a (large scale) simplified rename tool that allows renaming multiple things in a code base at once; it is definitely a useful tool to have.

Hm, I see. So if that's an often used scenario I'm all for it :)

Perhaps we can make this better by doing the following compromise:
Instead of just having tons of parameters where it's hard to say which parameter does what, have 2 different commands that you can call, each with their own set of flags:
clang-rename rename-at -offset ... -newname ...
clang-rename rename-all -oldname ... -newname ... -oldname ... -new-name ... (or similar)

This is probably similar to:

At the end I think we can both be happy if we separate the CLI and the renaming interface completely. The editors can interact with a C++ Library then and clang-rename would become a command line interface with many options to that library

So I guess I can probably get to that if this seems reasonable to you. I'd be happy to have a pure and clear interface, which won't deal with wrong user input etc.

Thanks for the feedback!

As far as I see tooling::CommonOptionsParser (in its current form) does not handle cl::SubCommand instances. Should I fix that or would it be OK to switch to using cl::ParseCommandLineOptions directly in clang-rename?

As far as I see tooling::CommonOptionsParser (in its current form) does not handle cl::SubCommand instances. Should I fix that or would it be OK to switch to using cl::ParseCommandLineOptions directly in clang-rename?

Well, if you switch, you'll lose a lot of what CommonOptionsParser gives you... So I think it's useful to keep it working with CommonOptionsParser.

The alternative is to change the CommonOptionsParser ctor to take a vector of OptionCategory&, and in that case we can add the -extra-arg-before and other options to all subcommands, but that means he'll have to adapt all client code, i.e. all tools in clang-tools-extra that uses CommonOptionsParser. Is that an acceptable cost? Or did I miss some easier way?

The alternative is to change the CommonOptionsParser ctor to take a vector of OptionCategory&, and in that case we can add the -extra-arg-before and other options to all subcommands, but that means he'll have to adapt all client code, i.e. all tools in clang-tools-extra that uses CommonOptionsParser. Is that an acceptable cost? Or did I miss some easier way?

So Subcommand doesn't support having a set of common options?

I'm a bit confused.

On one hand, I want to use tooling::CommonOptionsParser to parse the options, which needs a cl::OptionCategory as a parameter.

On the other hand, I want to parse the options, based on that I'll know what subcommand was requested, and then I can choose the right option category. Based on the above example, I want to use an option category that recognizes -oldname when "rename-all" is used, and I want an other option category that recognizes -offset" when "rename-at" is used.

I sense a chicken-and-egg problem, the argument of CommonOptionsParser needs the category, but I'll only know the correct category after I called CommonOptionsParser. Or is there a way out of this? ;-)

vmiklos updated this revision to Diff 64204.Jul 15 2016, 3:45 PM
vmiklos edited edge metadata.
vmiklos retitled this revision from clang-rename: support multiple renames with one invocation to clang-rename: split existing options into two new subcommands.
vmiklos updated this object.

I've implemented the requested split of options, now there are two new rename-at and rename-all subcommands. The only common options is -i and -new-name, nothing else is shared, apart from the common options added by tooling::CommonOptionsParser. The code is modeled after llvm-cov, that way I could break the chicken-and-egg problem I outlined in my previous comment.

I've also updated all tests to use either of the two subcommands.

For now I did not touch documentation, though that'll be obviously the next step, I just didn't want to make this patch larger than necessary. :-)

omtcyf0 removed a subscriber: omtcyf0.Jul 16 2016, 12:35 AM
  • Can you please update diff? I changed most of the tests recently.
  • I think you should update doc/clang-rename in this patch (making it a subsequent patch isn't worthy IMO)
  • Updating clang-rename/tool/clang-rename.py (simply add -rename-at into the argument list there) seems reasonable.
  • Also, I'd be happy to see at least few good tests for -rename-all with multiple -old-name and -new-name arguments.
  • Why does -rename-at not have -export-fixes option anymore?
  • Is there really a need to dispatch main to renameAtMain and renameAllMain? Most of the code is exactly the same (apart from YAML dump absence in renameAtMain, which I do not understand).
vmiklos updated this revision to Diff 64231.Jul 16 2016, 1:51 PM
  • Can you please update diff? I changed most of the tests recently.

Sure, I actually wanted to ask if those test additions were meant to be test
renames. :-)

  • I think you should update doc/clang-rename in this patch (making it a subsequent patch isn't worthy IMO)

Done.

  • Updating clang-rename/tool/clang-rename.py (simply add -rename-at into the argument list there) seems reasonable.

Done.

  • Also, I'd be happy to see at least few good tests for -rename-all with multiple -old-name and -new-name arguments.

Multiple -old-name / -new-name is not supported yet. I implemented that in the
first diff of this review, but then I was asked to split the two use cases into
separate subcommands first, and only support the multi-rename feature in
rename-all only. So I plan to add that in a subsequent patch. Or should squash
even that into this review?

  • Why does -rename-at not have -export-fixes option anymore?

The use-case for -export-fixes was that multiple translation units will want to
do the same replacements in common headers, so -i is not a good choice there.
Instead using -export-fixes, and then letting clang-apply-replacements do the
deduplication is the way to go. From this point of view, -export-fixes is not
useful for the rename-at / single TU use-case. But no problem, I've added it
back.

  • Is there really a need to dispatch main to renameAtMain and renameAllMain? Most of the code is exactly the same (apart from YAML dump absence in renameAtMain, which I do not understand).

The first idea was to use two separate binaries for rename-at/rename-all. Then
a compromise was to still have the same binary, but separate subcommands. So I
thought it's considered good to have a separate implementation of the separate
subcommands. But I'm happy if sharing code between rename-at and rename-all is
still OK, I've changed that.

  • Can you please update diff? I changed most of the tests recently.

Sure, I actually wanted to ask if those test additions were meant to be test
renames. :-)

Yeah, sorry for that...

  • I think you should update doc/clang-rename in this patch (making it a subsequent patch isn't worthy IMO)

Done.

  • Updating clang-rename/tool/clang-rename.py (simply add -rename-at into the argument list there) seems reasonable.

Done.

  • Also, I'd be happy to see at least few good tests for -rename-all with multiple -old-name and -new-name arguments.

Multiple -old-name / -new-name is not supported yet. I implemented that in the
first diff of this review, but then I was asked to split the two use cases into
separate subcommands first, and only support the multi-rename feature in
rename-all only. So I plan to add that in a subsequent patch. Or should squash
even that into this review?

Well, it might be fine for this one. Let's see what the others have to say.

  • Why does -rename-at not have -export-fixes option anymore?

The use-case for -export-fixes was that multiple translation units will want to
do the same replacements in common headers, so -i is not a good choice there.
Instead using -export-fixes, and then letting clang-apply-replacements do the
deduplication is the way to go. From this point of view, -export-fixes is not
useful for the rename-at / single TU use-case. But no problem, I've added it
back.

Ah, I can see your point. Well, there's still a long long way to the multi-TU stuff anyway... But I hope we'll get there at some point. I think both interfaces might be useful for multi-TU swell.

  • Is there really a need to dispatch main to renameAtMain and renameAllMain? Most of the code is exactly the same (apart from YAML dump absence in renameAtMain, which I do not understand).

The first idea was to use two separate binaries for rename-at/rename-all. Then
a compromise was to still have the same binary, but separate subcommands. So I
thought it's considered good to have a separate implementation of the separate
subcommands. But I'm happy if sharing code between rename-at and rename-all is
still OK, I've changed that.

Hm, I didn't think about it.

Well, honestly I'm not a fan of getting too many binaries and at the moment I think both interfaces are almost identical, so ATM I don't think we should get second binary, it will just make things even more complicated.

Is there anything I can help with to get this reviewed, please? As far as I see it still applies cleanly on top of current trunk.

Kirill, you happy with this approach?

The patch looks fine to me (though I'm not sure if there are no new tests; if they are interface changes should be applied).

If everyone seems to be in favor of such changes, I'm OK with it, but in general I think it makes things more complicated and I'm not sure if it's necessary at the moment; I expressed my ideas about it in comments to the other patch. But if that's what the common use-case is... So, TL;DR I personally don't see why one would want to rename multiple things at once while we still can't rename a single symbol correctly in too many cases...

P.S. it seems logical to me to support -offset option in -rename-all, too. And introducing -rename-all without actually supporting multiple renaming actions "at once" seems weird to me, too.

clang-rename/tool/ClangRename.cpp
250

use std::function here?

vmiklos updated this revision to Diff 64859.Jul 21 2016, 5:07 AM
vmiklos updated this object.
vmiklos marked an inline comment as done.

The patch looks fine to me (though I'm not sure if there are no new tests; if
they are interface changes should be applied).

make check-clang-tools + the patch at r276098 passes for me at least. But any
pending test should be trivial to adapt.

P.S. it seems logical to me to support -offset option in -rename-all,
too.

OK, I've added that, with a testcase.

And introducing -rename-all without actually supporting multiple
renaming actions "at once" seems weird to me, too.

OK, I've squashed the original diff into this one, with a testcase, which
addresses this.

A nice side-effect is that now the option parser takes care of checking if
-new-name is provided, no need to have explicit code for that in clang-rename.

use std::function here?

Done, also changed the typedef to using.

vmiklos updated this revision to Diff 64941.Jul 21 2016, 12:43 PM

Rebased on top of r276282 and resolved conflicts.

klimek added inline comments.Jul 21 2016, 11:06 PM
clang-rename/RenamingAction.cpp
48

Question is whether if we go down that route we shouldn't do one search for all the names, instead of re-looping for each name.

vmiklos added inline comments.Jul 22 2016, 12:45 AM
clang-rename/RenamingAction.cpp
48

You mean improving USRLocFindingASTVisitor, so that it can work with a list of USRs, not with just a single USR? I can do that, but if possible, I would like to do that as a follow-up patch; it was called in a loop already before this patch, so sounds orthogonal. (And it would potentially conflict with any pending patches that touch that class.)

I could fold HandleOneRename() into HandleTranslationUnit(), but that just makes the resulting function larger, and does not get rid of the looping. Or is that what you want?

Kirill, unless you have *specific* issues with this patch, I think it's good to land.

clang-rename/RenamingAction.cpp
48

No, the former. Can you add a FIXME?

vmiklos updated this revision to Diff 65031.Jul 22 2016, 12:55 AM
vmiklos marked an inline comment as done.

I'd be actually happy if instead of having -rename-at option we'd have this behavior by default unless -rename-all is used.

I'd be actually happy if instead of having -rename-at option we'd have this behavior by default unless -rename-all is used.

Not sure I understand this request. rename-at and rename-all all subcommands, not options. "have this by default", you mean the ability to perform multiple oldname->newname replacements with one commandline invocation? If so, not allowing that when the rename-all subcommand is chosen sounds confusing to me. But perhaps I misunderstand something. ;-)

I can make the rename-at subcommand optional, and when not specifying a subcommand, assume rename-at was specified (unless -help or -version is used). Is this what you want?

I can make the rename-at subcommand optional, and when not specifying a subcommand, assume rename-at was specified (unless -help or -version is used). Is this what you want?

Yep, exactly.

Sorry, I might have not expressed my idea good enough.

vmiklos updated this revision to Diff 65042.Jul 22 2016, 2:59 AM

Done, that also allows not modifying most existing tests.

vmiklos updated this revision to Diff 65417.Jul 25 2016, 2:02 PM

Rebased on top of r276684 and resolved conflicts.

Kirill, are your specific concerns addressed?

vmiklos updated this revision to Diff 65672.Jul 27 2016, 12:20 AM

Rebased on top of r276836 and resolved conflicts.

Apart from my high level concerns, which of course I still have...

clang-rename/tool/clang-rename.py
43

rename-at isn't necessary here anymore since it's going to be default behavior IIUC

docs/clang-rename.rst
35

docs should be fixed correspondingly; i.e. prefer to write clang-rename -offset=42.... over clang-rename -rename-at

50

here and later, too

vmiklos updated this revision to Diff 65684.Jul 27 2016, 2:06 AM
vmiklos updated this object.
vmiklos marked 2 inline comments as done.Jul 27 2016, 2:09 AM

rename-at isn't necessary here anymore since it's going to be default behavior IIUC

Indeed, it can be changed back now, done.

docs should be fixed correspondingly; i.e. prefer to write clang-rename -offset=42.... over clang-rename -rename-at

Done.

here and later, too

clang-rename rename-at is now only mentioned by the document when it comes to
clang-rename rename-at -help, which is still necessary, as `clang-rename
-help` talks about the subcommands only. I've updated clang-rename -help so
that it points out that rename-at is the default, though.

vmiklos updated this revision to Diff 65876.Jul 27 2016, 11:20 PM

Rebased on top of r276949 and resolved a failing test (FunctionWithClassFindByName.cpp).

Is there anything I can help with to get this accepted, please? As far as I see I addressed all so far mentioned concerns. Thanks.

omtcyfz added a subscriber: Eugene.Zelenko.EditedJul 29 2016, 2:04 AM
  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?

  1. 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).
  2. 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 does, too, from what I understand. Unless it's *Set, which is pretty much different thing.
  3. 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.

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

Feel free do disagree with my points, I may be terribly wrong :)

vmiklos updated this revision to Diff 66099.Jul 29 2016, 2:40 AM
vmiklos added a subscriber: Eugene.Zelenko.
  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. :)

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

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

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

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

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

  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.

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

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

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

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

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

besides, let me push one thing; it's about passing a vector of USRs to the USRLocFinder instead of passing them 1 by 1; removes a need to write that FIXME of yours :)

Hm, nevermind, I should check whether it doesn't break anything.

Apparently it doesn't. Pushed to upstream.

vmiklos updated this revision to Diff 66105.Jul 29 2016, 3:59 AM

Rebased on top of r277131 and resolved conflicts.

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

I think so; we have this chicken-and-egg problem (see earlier comments of this
review), that the options parser wants to know the option category, but we only
know the option category after parsing options due to subcommands. This is not
a problem for clang-tidy that doesn't have subcommands (as far as I see).

So one way is the own code for handling the subcommands (that's what I did
here, and that's what e.g. llvm-cov does), an other way would be to extend
tooling::CommonOptionsParser, so it doesn't want a category in the ctor. That
requirement is a problem for us, since we have two categories, so we can't give
the correct one without parsing the options.

So all in all, the best seems to me is to go with a simple helpMain().

besides, let me push one thing; it's about passing a vector of USRs to the USRLocFinder instead of passing them 1 by 1; removes a need to write that FIXME of yours :)

Great, I've removed it then.

Rebased on top of r277131 and resolved conflicts.

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

I think so; we have this chicken-and-egg problem (see earlier comments of this
review), that the options parser wants to know the option category, but we only
know the option category after parsing options due to subcommands. This is not
a problem for clang-tidy that doesn't have subcommands (as far as I see).

So one way is the own code for handling the subcommands (that's what I did
here, and that's what e.g. llvm-cov does), an other way would be to extend
tooling::CommonOptionsParser, so it doesn't want a category in the ctor. That
requirement is a problem for us, since we have two categories, so we can't give
the correct one without parsing the options.

So all in all, the best seems to me is to go with a simple helpMain().

Aha. I see. Well, it's not clean, but we can probably just deal with it later. Otherwise it gets too long and the patch is never going to be landed. Just write a FIXME then, I think I may look into that on the next week or somewhen.

besides, let me push one thing; it's about passing a vector of USRs to the USRLocFinder instead of passing them 1 by 1; removes a need to write that FIXME of yours :)

Great, I've removed it then.

One more paranoid comment:

class Cla1 { // CHECK: class Kla1

Most of the time I use Foo->Bar renaming in tests, just so that it could be uniform and everyone (including myself) would typically grep -FUbo "Foo" without looking at the source.

Other than that my concerns have been addressed.

Again, I'm not happy about this approach, but if that's a use-case we expect to have in the future and everyone is happy about it - let's do that.

vmiklos updated this revision to Diff 66110.Jul 29 2016, 5:27 AM

Just write a FIXME then, I think I may look into that on the next week or somewhen.

Done.

Most of the time I use Foo->Bar renaming in tests

Done, I've renamed ClaN->KlaN to FooN->BarN.

P.S. not sure whether we have to write clang-rename: for the -new-name option: must be specified out. We already launched clang-rename what else could've give us an error?

P.S. not sure whether we have to write clang-rename: for the -new-name option: must be specified out. We already launched clang-rename what else could've give us an error?

You mean how is that error produced? Previously there was no cl::Required on NewName (now: NewNames), and that's why a manual check was needed, the error message changed, as now the option parser does this checking for us.

P.S. not sure whether we have to write clang-rename: for the -new-name option: must be specified out. We already launched clang-rename what else could've give us an error?

You mean how is that error produced? Previously there was no cl::Required on NewName (now: NewNames), and that's why a manual check was needed, the error message changed, as now the option parser does this checking for us.

Ah, I see. So the message is generated, isn't it?

Yes, exactly, so not easy to customize I guess.

Yes, exactly, so not easy to customize I guess.

Aha, alright. Well, doesn't matter too much.

LGTM.

Great! Manuel, OK to land?

vmiklos updated this revision to Diff 66304.Aug 1 2016, 5:06 AM

Rebased on top of r277339 and resolved conflicts.

vmiklos updated this revision to Diff 66362.Aug 1 2016, 1:03 PM

Rebased on top of r277356 and resolved conflicts. (A busy day, it seems. :-) )

klimek accepted this revision.Aug 2 2016, 2:20 AM
klimek edited edge metadata.

lg

This revision is now accepted and ready to land.Aug 2 2016, 2:20 AM

Make sure to rebase once more; documentation was updated in the last revision.

This revision was automatically updated to reflect the committed changes.

Yes, I did that -- but I got no conflicts there. ;-)