[refactor] add support for refactoring options
ClosedPublic

Authored by arphaman on Sep 14 2017, 11:06 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

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

include/clang/Tooling/Refactoring/RefactoringActionRule.h
48 ↗(On Diff #115248)

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

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
62 ↗(On Diff #115248)

that a used?

78 ↗(On Diff #115248)

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

83 ↗(On Diff #115248)

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

88 ↗(On Diff #115248)

Please explain why shared_ptr is needed here.

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
56 ↗(On Diff #115248)

nit: no braces around one liner in LLVM style.

68 ↗(On Diff #115248)

Please elaborate on what's valid or invalid.

74 ↗(On Diff #115248)

Why shared_ptr? Would unique_ptr work?

include/clang/Tooling/Refactoring/RefactoringOption.h
42 ↗(On Diff #115248)

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

47 ↗(On Diff #115248)

Again, why shared_ptr instead of unique_ptr?

include/clang/Tooling/Refactoring/RefactoringOptionConsumer.h
26 ↗(On Diff #115248)

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

tools/clang-refactor/ClangRefactor.cpp
372 ↗(On Diff #115248)

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

arphaman updated this revision to Diff 117124.Fri, Sep 29, 4:58 AM
arphaman marked 10 inline comments as done.

Address review comments

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
78 ↗(On Diff #115248)

To support more complex requirements that need multiple options.

83 ↗(On Diff #115248)

I've simplified this by using a typealias in the option itself.

88 ↗(On Diff #115248)

The same option can be used by multiple rules, hence options can be owned by multiple requirements in different rules at once. I documented this.

include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
68 ↗(On Diff #115248)

Oops, copy-pasted comment. I fixed it up.

74 ↗(On Diff #115248)

I refactored this to change the redundant ownership here.

tools/clang-refactor/ClangRefactor.cpp
372 ↗(On Diff #115248)

I think right now it's tricky because of the testing support. However, I think that it would be better to use a similar mechanism for selection option as well. I'll think about how we can reconcile the two approaches and will hopefully come up with a patch for it.

ioeric accepted this revision.Sun, Oct 1, 3:31 PM

Looks good with some nits.

include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
73 ↗(On Diff #117124)

nit: OptionRequirement sounds more general than the base class RefactoringOptionsRequirement.

include/clang/Tooling/Refactoring/RefactoringOption.h
47 ↗(On Diff #117124)

nit: s/Consumer/Visitor/

tools/clang-refactor/ClangRefactor.cpp
129 ↗(On Diff #117124)

We use templated method for adding options above but type-specific interfaces (e.g. getStringOption) for getting options. I think we should make either both templated or separate interfaces. I am a bit inclined to separate interfaces for each type for clarity.

149 ↗(On Diff #117124)

Visitor?

This revision is now accepted and ready to land.Sun, Oct 1, 3:31 PM
This revision was automatically updated to reflect the committed changes.
arphaman marked 3 inline comments as done.
arphaman added inline comments.Fri, Oct 6, 11:14 AM
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
73 ↗(On Diff #117124)

I couldn't really think of a better name, sorry.

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

cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
113

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.

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.

cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
113

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.

hokein added inline comments.Wed, Oct 11, 9:25 AM
cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
113

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".