[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action
Eric Liu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 6 05:38:55 PDT 2017
ioeric added inline comments.
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:103
+ IsSelectionParsed = true;
+ // FIXME: Support true selection ranges.
+ StringRef Value = *Selection;
----------------
arphaman wrote:
> ioeric wrote:
> > Is the test selection temporary before we have true selection?
> >
> > I am a bit concerned about having test logic in the production code. It makes the code paths a bit complicated, and we might easily end up testing the test logic instead of the actual code logic. I'm fine with this if this is just a short term solution before we have the actual selection support.
> No, both are meant to co-exist. I guess we could introduce a new option (-selection vs -test-selection and hide -test-selection)? Another approach that I was thinking about is to have a special hidden subcommand for each action (e.g. test-local-rename). Alternatively we could use two different tools (clang-refactor vs clang-refactor-test)? Both will have very similar code though.
>
> Note that it's not the first time test logic will exist in the final tool. For example, llvm-cov has a `convert-for-testing` subcommand that exists in the production tool. I'm not saying that it's necessarily the best option though, but I don't think it's the worst one either ;)
I guess I am more concerned about mixing production code with testing code than having special options. For example, we have different invocation paths based on `ParsedTestSelection`. IMO, whether selected ranges come from test selection or actual selection should be transparent to clang-refactor. Maybe refactoring the selection handling logic out as a separate interface would help?
Repository:
rL LLVM
https://reviews.llvm.org/D36574
More information about the cfe-commits
mailing list