ioeric added inline comments.
================
Comment at: include/clang/Tooling/Refactoring/RefactoringAction.h:20
+
+class RefactoringAction {
+public:
----------------
Please add documentation for the class.
I appreciate your RFC that explains all these concepts; it would be nice if you
could move some key concepts into the code documentation since we couldn't
expect future readers to refer to the RFC. Thanks! ;)
================
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:42
+/// This constraint is satisfied when
+template <typename T> struct Decl {
----------------
when?
================
Comment at: include/clang/Tooling/Refactoring/SourceSelectionConstraints.h:55
/// Subclasses must implement 'T evaluateSelection(SelectionConstraint)
const'
- /// member function. \c T is used to determine the return type that is
- /// passed to the refactoring rule's function.
+ /// or 'T evaluateSelection(const ASTRefactoringContext &,
+ /// SelectionConstraint) const' member function. \c T is used to determine
----------------
Can we pass `RefactoringRuleContext` instead of `ASTRefactoringContext` and get
rules can get AST context from the rule context? I'm a bit nervous about having
different contexts.
Maybe we should have just one interface `T
evaluateSelection(RefactoringRuleContext, SelectionConstraint) const`. I think
the rule context can be useful for all rules.
================
Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:35
using namespace llvm;
+using namespace clang;
----------------
nit: It's a common practice to wrap implementations in namespaces instead of
using `using namespace`. It becomes hard to manage scopes/namespaces when there
are `using namespace`s.
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:60
+ : SubCommand(Name, Description), Action(&Action) {
+ Sources = llvm::make_unique<cl::list<std::string>>(
+ cl::Positional, cl::ZeroOrMore, cl::desc("<source0> [... <sourceN>]"),
----------------
I think you would get a conflict of positional args when you have multiple
sub-command instances.
Any reason not to use `clang::tooling::CommonOptionsParser` which takes care of
sources and compilation options for you?
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:312
+ [](const std::unique_ptr<RefactoringActionSubcommand> &SubCommand) {
+ return !!(*SubCommand);
+ });
----------------
Do we need a `FIXME` here? It seems that this simply finds the first command
that is available? What if there are more than one commands that satisfy the
requirements?
================
Comment at: tools/clang-refactor/ClangRefactor.cpp:72
+ if (HasSelection) {
+ Selection = llvm::make_unique<cl::opt<std::string>>(
+ "selection", cl::desc("Source selection range"), cl::cat(Category),
----------------
Same here. There could be potential conflict among subcommands.
Also please provide more detailed description of this option; it is not obvious
to users what a `selection` is.
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
