arphaman added inline comments.
================ Comment at: tools/clang-refactor/ClangRefactor.cpp:103 + IsSelectionParsed = true; + // FIXME: Support true selection ranges. + StringRef Value = *Selection; ---------------- 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 ;) Repository: rL LLVM https://reviews.llvm.org/D36574 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
