[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

[Eric Liu via Phabricator via cfe-commits]

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