[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 04:55:01 PDT 2016


omtcyfz added a comment.

In https://reviews.llvm.org/D21814#500506, @vmiklos wrote:

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


https://reviews.llvm.org/D21814





More information about the cfe-commits mailing list