[PATCH] D37856: [refactor] add support for refactoring options

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113
 Rules.push_back(createRefactoringActionRule(
-SymbolSelectionRequirement()));
+SymbolSelectionRequirement(), OptionRequirement()));
 return Rules;

arphaman wrote:
> hokein wrote:
> > Thought it a bit more: it requires all of the requirements are satisfied, I 
> > think we need to support "one-of" option. For example,  we have two option 
> > "-a" and "-b",  only one of them is allowed to be present at the same time.
> That should be straightforward enough to implement, either with a custom 
> class or with a built-in requirement class. I'll probably add a builtin one 
> when the need comes up.
Thanks. I'm asking it because I plan to add the `-qualified-name` option to the 
clang-refactor rename subtool. And obviously "-qualified-name" can not be 
present with "-selection".


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37856: [refactor] add support for refactoring options

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D37856#894638, @hokein wrote:

> Sorry for the delay. I saw you have reverted this commit somehow. A post 
> commit.


I had some issues with ppc64/s390x bots for some reason, so I had to revert. 
I'm still trying to investigate what went wrong there, it looks like ASAN/UBSAN 
are not catching anything though.




Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113
 Rules.push_back(createRefactoringActionRule(
-SymbolSelectionRequirement()));
+SymbolSelectionRequirement(), OptionRequirement()));
 return Rules;

hokein wrote:
> Thought it a bit more: it requires all of the requirements are satisfied, I 
> think we need to support "one-of" option. For example,  we have two option 
> "-a" and "-b",  only one of them is allowed to be present at the same time.
That should be straightforward enough to implement, either with a custom class 
or with a built-in requirement class. I'll probably add a builtin one when the 
need comes up.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37856: [refactor] add support for refactoring options

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Sorry for the delay. I saw you have reverted this commit somehow. A post commit.




Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113
 Rules.push_back(createRefactoringActionRule(
-SymbolSelectionRequirement()));
+SymbolSelectionRequirement(), OptionRequirement()));
 return Rules;

Thought it a bit more: it requires all of the requirements are satisfied, I 
think we need to support "one-of" option. For example,  we have two option "-a" 
and "-b",  only one of them is allowed to be present at the same time.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37856: [refactor] add support for refactoring options

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:73
+template 
+class OptionRequirement : public RefactoringOptionsRequirement {
+public:

ioeric wrote:
> nit: `OptionRequirement` sounds more general than the base class 
> `RefactoringOptionsRequirement`. 
I couldn't really think of a better name, sorry.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37856: [refactor] add support for refactoring options

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 117124.
arphaman marked 10 inline comments as done.
arphaman added a comment.

Address review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D37856

Files:
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringOption.h
  include/clang/Tooling/Refactoring/RefactoringOptionVisitor.h
  include/clang/Tooling/Refactoring/RefactoringOptions.h
  include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  test/Refactor/LocalRename/Field.cpp
  test/Refactor/tool-test-support.c
  tools/clang-refactor/ClangRefactor.cpp

Index: tools/clang-refactor/ClangRefactor.cpp
===
--- tools/clang-refactor/ClangRefactor.cpp
+++ tools/clang-refactor/ClangRefactor.cpp
@@ -18,6 +18,7 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Refactoring/RefactoringAction.h"
+#include "clang/Tooling/Refactoring/RefactoringOptions.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
@@ -32,10 +33,10 @@
 
 namespace opts {
 
-static cl::OptionCategory CommonRefactorOptions("Common refactoring options");
+static cl::OptionCategory CommonRefactorOptions("Refactoring options");
 
 static cl::opt Verbose("v", cl::desc("Use verbose output"),
- cl::cat(CommonRefactorOptions),
+ cl::cat(cl::GeneralCategory),
  cl::sub(*cl::AllSubCommands));
 } // end namespace opts
 
@@ -116,6 +117,98 @@
   return nullptr;
 }
 
+/// A container that stores the command-line options used by a single
+/// refactoring option.
+class RefactoringActionCommandLineOptions {
+public:
+  template 
+  void addOption(const RefactoringOption ,
+ std::unique_ptr CLOption);
+
+  const cl::opt &
+  getStringOption(const RefactoringOption ) const {
+auto It = StringOptions.find();
+return *It->second;
+  }
+
+private:
+  llvm::DenseMap>>
+  StringOptions;
+};
+
+template <>
+void RefactoringActionCommandLineOptions::addOption(
+const RefactoringOption ,
+std::unique_ptr CLOption) {
+  StringOptions[] = std::move(CLOption);
+}
+
+/// Passes the command-line option values to the options used by a single
+/// refactoring action rule.
+class CommandLineRefactoringOptionConsumer final
+: public RefactoringOptionVisitor {
+public:
+  CommandLineRefactoringOptionConsumer(
+  const RefactoringActionCommandLineOptions )
+  : Options(Options) {}
+
+  void visit(const RefactoringOption ,
+ Optional ) override {
+const cl::opt  = Options.getStringOption(Opt);
+if (!CLOpt.getValue().empty()) {
+  Value = CLOpt.getValue();
+  return;
+}
+Value = None;
+if (Opt.isRequired())
+  MissingRequiredOptions.push_back();
+  }
+
+  ArrayRef getMissingRequiredOptions() const {
+return MissingRequiredOptions;
+  }
+
+private:
+  llvm::SmallVector MissingRequiredOptions;
+  const RefactoringActionCommandLineOptions 
+};
+
+/// Creates the refactoring options used by all the rules in a single
+/// refactoring action.
+class CommandLineRefactoringOptionCreator final
+: public RefactoringOptionVisitor {
+public:
+  CommandLineRefactoringOptionCreator(
+  cl::OptionCategory , cl::SubCommand ,
+  RefactoringActionCommandLineOptions )
+  : Category(Category), Subcommand(Subcommand), Options(Options) {}
+
+  void visit(const RefactoringOption , Optional &) override {
+if (Visited.insert().second)
+  Options.addOption(Opt, create(Opt));
+  }
+
+private:
+  template 
+  std::unique_ptr create(const RefactoringOption ) {
+if (!OptionNames.insert(Opt.getName()).second)
+  llvm::report_fatal_error("Multiple identical refactoring options "
+   "specified for one refactoring action");
+// FIXME: cl::Required can be specified when this option is present
+// in all rules in an action.
+return llvm::make_unique(
+Opt.getName(), cl::desc(Opt.getDescription()), cl::Optional,
+cl::cat(Category), cl::sub(Subcommand));
+  }
+
+  llvm::SmallPtrSet Visited;
+  llvm::StringSet<> OptionNames;
+  cl::OptionCategory 
+  cl::SubCommand 
+  RefactoringActionCommandLineOptions 
+};
+
 /// A subcommand that corresponds to individual refactoring action.
 class RefactoringActionSubcommand : public cl::SubCommand {
 public:
@@ -138,6 +231,12 @@
"::)"),
   cl::cat(Category), cl::sub(*this));
 }
+// Create the refactoring options.
+for (const auto  : this->ActionRules) {
+  

[PATCH] D37856: [refactor] add support for refactoring options

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Sorry for the delay (most of us are OOO this week).




Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:48
+
+  virtual void visitRefactoringOptions(RefactoringOptionConsumer ) = 
0;
 };

Please document the behavior here. What options are visited? How is the 
consumer used?



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:62
+
+  /// Returns the set of refactoring options that a used when evaluating this
+  /// requirement.

`that a used`?



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:78
+  std::vector
+  getRefactoringOptions() const final override {
+return {Opt};

Why return a vector instead of a single value if there is only one element?



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:83
+  Expected
+  evaluate(RefactoringRuleContext &) const {
+return Opt->getValue();

This could use some comment, e.g. what is `getValue` expected to do?



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:88
+private:
+  std::shared_ptr Opt;
+};

Please explain why `shared_ptr` is needed here.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:56
   auto Err = findError(std::get(Values)...);
-  if (Err)
+  if (Err) {
 return Consumer.handleError(std::move(Err));

nit: no braces around one liner in LLVM style.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:68
+/// Scans the tuple and returns a valid \c Error if any of the values are
+/// invalid.
+template 

Please elaborate on what's valid or invalid.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:74
+  struct OptionGatherer {
+std::vector 
+

Why `shared_ptr`? Would `unique_ptr` work?



Comment at: include/clang/Tooling/Refactoring/RefactoringOption.h:42
+
+  virtual void passToConsumer(RefactoringOptionConsumer ) = 0;
+};

This could also use some comment. E.g. what is passed/cosumed? 



Comment at: include/clang/Tooling/Refactoring/RefactoringOption.h:47
+template 
+std::shared_ptr createRefactoringOption() {
+  static_assert(std::is_base_of::value,

Again, why `shared_ptr` instead of `unique_ptr`?



Comment at: include/clang/Tooling/Refactoring/RefactoringOptionConsumer.h:26
+/// declaration in this interface.
+class RefactoringOptionConsumer {
+public:

IIUC, you are using visitor pattern to "handle" options. If so, consider 
`s/consumer/visitor` and `s/handle/visit`, just to be clearer.



Comment at: tools/clang-refactor/ClangRefactor.cpp:372
   if (Rule->hasSelectionRequirement()) {
 HasSelection = true;
+if (!Subcommand.getSelection()) {

Wondering if it is sensible to make `selection` also a general option so that 
we don't need to special-case selections.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37856: [refactor] add support for refactoring options

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.

This patch adds initial support for refactoring options. One can now use 
optional and required `std::string` options.

The options are implemented in the following manner:

- A base interface `RefactoringOption` declares methods that describe the 
option's name, description and whether or not it's required. It also declares a 
method that takes in a `RefactoringOptionConsumer` that will visit the option 
and its typed value.
- A visitor interface `RefactoringOptionConsumer` declares the `handle` methods 
that provide an overload for each valid option type (std::string, etc).
- Classes `OptionalRequiredOption` and `RequiredRefactoringOption` partially 
implement the `RefactoringOption` interface, store the value and provide a 
getter for the value. The individual options then just have to implement 
`getName` and `getDescription`.
- The `RefactoringOptionsRequirement` is a base requirement class the declares 
a method that can returns all of the options used in a single requirement.
- The `OptionRequirement` class is a helper class that implements a refactoring 
action rule requirement for a particular option.

`clang-refactor` now creates command-line arguments for each unique option used 
by all rules in an action.

This patch also adds a `NewNameOption` for the local-rename refactoring action 
to ensure that options work.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856

Files:
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringOption.h
  include/clang/Tooling/Refactoring/RefactoringOptionConsumer.h
  include/clang/Tooling/Refactoring/RefactoringOptions.h
  include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  test/Refactor/LocalRename/Field.cpp
  test/Refactor/tool-test-support.c
  tools/clang-refactor/ClangRefactor.cpp

Index: tools/clang-refactor/ClangRefactor.cpp
===
--- tools/clang-refactor/ClangRefactor.cpp
+++ tools/clang-refactor/ClangRefactor.cpp
@@ -18,6 +18,7 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Refactoring/RefactoringAction.h"
+#include "clang/Tooling/Refactoring/RefactoringOptions.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
@@ -32,10 +33,10 @@
 
 namespace opts {
 
-static cl::OptionCategory CommonRefactorOptions("Common refactoring options");
+static cl::OptionCategory CommonRefactorOptions("Refactoring options");
 
 static cl::opt Verbose("v", cl::desc("Use verbose output"),
- cl::cat(CommonRefactorOptions),
+ cl::cat(cl::GeneralCategory),
  cl::sub(*cl::AllSubCommands));
 } // end namespace opts
 
@@ -116,6 +117,98 @@
   return nullptr;
 }
 
+/// A container that stores the command-line options used by a single
+/// refactoring option.
+class RefactoringActionCommandLineOptions {
+public:
+  template 
+  void addOption(const RefactoringOption ,
+ std::unique_ptr CLOption);
+
+  const cl::opt &
+  getStringOption(const RefactoringOption ) const {
+auto It = StringOptions.find();
+return *It->second;
+  }
+
+private:
+  llvm::DenseMap>>
+  StringOptions;
+};
+
+template <>
+void RefactoringActionCommandLineOptions::addOption(
+const RefactoringOption ,
+std::unique_ptr CLOption) {
+  StringOptions[] = std::move(CLOption);
+}
+
+/// Passes the command-line option values to the options used by a single
+/// refactoring action rule.
+class CommandLineRefactoringOptionConsumer final
+: public RefactoringOptionConsumer {
+public:
+  CommandLineRefactoringOptionConsumer(
+  const RefactoringActionCommandLineOptions )
+  : Options(Options) {}
+
+  void handle(const RefactoringOption ,
+  Optional ) override {
+const cl::opt  = Options.getStringOption(Opt);
+if (!CLOpt.getValue().empty()) {
+  Value = CLOpt.getValue();
+  return;
+}
+Value = None;
+if (Opt.isRequired())
+  MissingRequiredOptions.push_back();
+  }
+
+  ArrayRef getMissingRequiredOptions() const {
+return MissingRequiredOptions;
+  }
+
+private:
+  llvm::SmallVector MissingRequiredOptions;
+  const RefactoringActionCommandLineOptions 
+};
+
+/// Creates the refactoring options used by all the rules in a single
+/// refactoring action.
+class CommandLineRefactoringOptionCreator final
+: public RefactoringOptionConsumer {
+public:
+  CommandLineRefactoringOptionCreator(
+  cl::OptionCategory , cl::SubCommand ,
+  RefactoringActionCommandLineOptions )
+  : Category(Category), Subcommand(Subcommand),