arphaman marked an inline comment as done.
arphaman added inline comments.
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:79
+
+ RefactoringAction &getAction() const { return *Action; }
+
----------------
ioeric wrote:
> Why return a mutable reference? Also, this is not used in this patch; maybe
> leave it out?
Removed.
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:103
+ IsSelectionParsed = true;
+ // FIXME: Support true selection ranges.
+ StringRef Value = *Selection;
----------------
ioeric wrote:
> 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?
Makes sense, I just did a cleanup refactoring that should make it better.
Repository:
rL LLVM
https://reviews.llvm.org/D36574
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
