From: Alex Lorenz Date: Fri, 6 Oct 2017 19:49:29 +0000 (+0000) Subject: Revert r315087 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f1381a926e22e996f38e74fd74bc9a25dfbac8f9;p=clang Revert r315087 clang-refactor crashes on some bots after this commit git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@315095 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Tooling/Refactoring/RefactoringActionRule.h b/include/clang/Tooling/Refactoring/RefactoringActionRule.h index 294ccc381b..c72b37c91d 100644 --- a/include/clang/Tooling/Refactoring/RefactoringActionRule.h +++ b/include/clang/Tooling/Refactoring/RefactoringActionRule.h @@ -16,7 +16,6 @@ namespace clang { namespace tooling { -class RefactoringOptionVisitor; class RefactoringResultConsumer; class RefactoringRuleContext; @@ -44,14 +43,6 @@ public: /// Returns true when the rule has a source selection requirement that has /// to be fullfilled before refactoring can be performed. virtual bool hasSelectionRequirement() = 0; - - /// Traverses each refactoring option used by the rule and invokes the - /// \c visit callback in the consumer for each option. - /// - /// Options are visited in the order of use, e.g. if a rule has two - /// requirements that use options, the options from the first requirement - /// are visited before the options in the second requirement. - virtual void visitRefactoringOptions(RefactoringOptionVisitor &Visitor) = 0; }; } // end namespace tooling diff --git a/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h b/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h index 0a4ee8e997..ebfeeda589 100644 --- a/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h +++ b/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h @@ -11,7 +11,6 @@ #define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_REQUIREMENTS_H #include "clang/Basic/LLVM.h" -#include "clang/Tooling/Refactoring/RefactoringOption.h" #include "clang/Tooling/Refactoring/RefactoringRuleContext.h" #include "llvm/Support/Error.h" #include @@ -54,45 +53,6 @@ public: } }; -/// A base class for any requirement that requires some refactoring options. -class RefactoringOptionsRequirement : public RefactoringActionRuleRequirement { -public: - virtual ~RefactoringOptionsRequirement() {} - - /// Returns the set of refactoring options that are used when evaluating this - /// requirement. - virtual ArrayRef> - getRefactoringOptions() const = 0; -}; - -/// A requirement that evaluates to the value of the given \c OptionType when -/// the \c OptionType is a required option. When the \c OptionType is an -/// optional option, the requirement will evaluate to \c None if the option is -/// not specified or to an appropriate value otherwise. -template -class OptionRequirement : public RefactoringOptionsRequirement { -public: - OptionRequirement() : Opt(createRefactoringOption()) {} - - ArrayRef> - getRefactoringOptions() const final override { - return static_cast &>(Opt); - } - - Expected - evaluate(RefactoringRuleContext &) const { - return Opt->getValue(); - } - -private: - /// The partially-owned option. - /// - /// The ownership of the option is shared among the different requirements - /// because the same option can be used by multiple rules in one refactoring - /// action. - std::shared_ptr Opt; -}; - } // end namespace tooling } // end namespace clang diff --git a/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h b/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h index cf6147c0ba..61db7400ac 100644 --- a/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h +++ b/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h @@ -24,23 +24,12 @@ namespace internal { inline llvm::Error findError() { return llvm::Error::success(); } -inline void ignoreError() {} - -template -void ignoreError(Expected &First, Expected &... Rest) { - if (!First) - llvm::consumeError(First.takeError()); - ignoreError(Rest...); -} - /// Scans the tuple and returns a valid \c Error if any of the values are /// invalid. template llvm::Error findError(Expected &First, Expected &... Rest) { - if (!First) { - ignoreError(Rest...); + if (!First) return First.takeError(); - } return findError(Rest...); } @@ -60,34 +49,6 @@ void invokeRuleAfterValidatingRequirements( RuleType((*std::get(Values))...).invoke(Consumer, Context); } -inline void visitRefactoringOptionsImpl(RefactoringOptionVisitor &) {} - -/// Scans the list of requirements in a rule and visits all the refactoring -/// options that are used by all the requirements. -template -void visitRefactoringOptionsImpl(RefactoringOptionVisitor &Visitor, - const FirstT &First, const RestT &... Rest) { - struct OptionGatherer { - RefactoringOptionVisitor &Visitor; - - void operator()(const RefactoringOptionsRequirement &Requirement) { - for (const auto &Option : Requirement.getRefactoringOptions()) - Option->passToVisitor(Visitor); - } - void operator()(const RefactoringActionRuleRequirement &) {} - }; - (OptionGatherer{Visitor})(First); - return visitRefactoringOptionsImpl(Visitor, Rest...); -} - -template -void visitRefactoringOptions( - RefactoringOptionVisitor &Visitor, - const std::tuple &Requirements, - llvm::index_sequence) { - visitRefactoringOptionsImpl(Visitor, std::get(Requirements)...); -} - /// A type trait that returns true when the given type list has at least one /// type whose base is the given base type. template @@ -136,12 +97,6 @@ createRefactoringActionRule(const RequirementTypes &... Requirements) { RequirementTypes...>::value; } - void visitRefactoringOptions(RefactoringOptionVisitor &Visitor) override { - internal::visitRefactoringOptions( - Visitor, Requirements, - llvm::index_sequence_for()); - } - private: std::tuple Requirements; }; diff --git a/include/clang/Tooling/Refactoring/RefactoringOption.h b/include/clang/Tooling/Refactoring/RefactoringOption.h deleted file mode 100644 index 5011223cce..0000000000 --- a/include/clang/Tooling/Refactoring/RefactoringOption.h +++ /dev/null @@ -1,64 +0,0 @@ -//===--- RefactoringOption.h - Clang refactoring library ------------------===// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTION_H -#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTION_H - -#include "clang/Basic/LLVM.h" -#include -#include - -namespace clang { -namespace tooling { - -class RefactoringOptionVisitor; - -/// A refactoring option is an interface that describes a value that -/// has an impact on the outcome of a refactoring. -/// -/// Refactoring options can be specified using command-line arguments when -/// the clang-refactor tool is used. -class RefactoringOption { -public: - virtual ~RefactoringOption() {} - - /// Returns the name of the refactoring option. - /// - /// Each refactoring option must have a unique name. - virtual StringRef getName() const = 0; - - virtual StringRef getDescription() const = 0; - - /// True when this option must be specified before invoking the refactoring - /// action. - virtual bool isRequired() const = 0; - - /// Invokes the \c visit method in the option consumer that's appropriate - /// for the option's value type. - /// - /// For example, if the option stores a string value, this method will - /// invoke the \c visit method with a reference to an std::string value. - virtual void passToVisitor(RefactoringOptionVisitor &Visitor) = 0; -}; - -/// Constructs a refactoring option of the given type. -/// -/// The ownership of options is shared among requirements that use it because -/// one option can be used by multiple rules in a refactoring action. -template -std::shared_ptr createRefactoringOption() { - static_assert(std::is_base_of::value, - "invalid option type"); - return std::make_shared(); -} - -} // end namespace tooling -} // end namespace clang - -#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTION_H diff --git a/include/clang/Tooling/Refactoring/RefactoringOptionVisitor.h b/include/clang/Tooling/Refactoring/RefactoringOptionVisitor.h deleted file mode 100644 index aea8fa5493..0000000000 --- a/include/clang/Tooling/Refactoring/RefactoringOptionVisitor.h +++ /dev/null @@ -1,62 +0,0 @@ -//===--- RefactoringOptionVisitor.h - Clang refactoring library -----------===// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTION_VISITOR_H -#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTION_VISITOR_H - -#include "clang/Basic/LLVM.h" -#include - -namespace clang { -namespace tooling { - -class RefactoringOption; - -/// An interface that declares functions that handle different refactoring -/// option types. -/// -/// A valid refactoring option type must have a corresponding \c visit -/// declaration in this interface. -class RefactoringOptionVisitor { -public: - virtual ~RefactoringOptionVisitor() {} - - virtual void visit(const RefactoringOption &Opt, - Optional &Value) = 0; -}; - -namespace traits { -namespace internal { - -template struct HasHandle { -private: - template - static auto check(ClassT *) -> typename std::is_same< - decltype(std::declval().visit( - std::declval(), *std::declval *>())), - void>::type; - - template static std::false_type check(...); - -public: - using Type = decltype(check(nullptr)); -}; - -} // end namespace internal - -/// A type trait that returns true iff the given type is a type that can be -/// stored in a refactoring option. -template -struct IsValidOptionType : internal::HasHandle::Type {}; - -} // end namespace traits -} // end namespace tooling -} // end namespace clang - -#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTION_VISITOR_H diff --git a/include/clang/Tooling/Refactoring/RefactoringOptions.h b/include/clang/Tooling/Refactoring/RefactoringOptions.h deleted file mode 100644 index e45c0a09fd..0000000000 --- a/include/clang/Tooling/Refactoring/RefactoringOptions.h +++ /dev/null @@ -1,58 +0,0 @@ -//===--- RefactoringOptions.h - Clang refactoring library -----------------===// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTIONS_H -#define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTIONS_H - -#include "clang/Basic/LLVM.h" -#include "clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h" -#include "clang/Tooling/Refactoring/RefactoringOption.h" -#include "clang/Tooling/Refactoring/RefactoringOptionVisitor.h" -#include "llvm/Support/Error.h" -#include - -namespace clang { -namespace tooling { - -/// A refactoring option that stores a value of type \c T. -template ::value>::type> -class OptionalRefactoringOption : public RefactoringOption { -public: - void passToVisitor(RefactoringOptionVisitor &Visitor) final override { - Visitor.visit(*this, Value); - } - - bool isRequired() const override { return false; } - - using ValueType = Optional; - - const ValueType &getValue() const { return Value; } - -protected: - Optional Value; -}; - -/// A required refactoring option that stores a value of type \c T. -template ::value>::type> -class RequiredRefactoringOption : public OptionalRefactoringOption { -public: - using ValueType = T; - - const ValueType &getValue() const { - return *OptionalRefactoringOption::Value; - } - bool isRequired() const final override { return true; } -}; - -} // end namespace tooling -} // end namespace clang - -#endif // LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_OPTIONS_H diff --git a/include/clang/Tooling/Refactoring/Rename/RenamingAction.h b/include/clang/Tooling/Refactoring/Rename/RenamingAction.h index f2d9a7bb4d..6f67287084 100644 --- a/include/clang/Tooling/Refactoring/Rename/RenamingAction.h +++ b/include/clang/Tooling/Refactoring/Rename/RenamingAction.h @@ -17,7 +17,6 @@ #include "clang/Tooling/Refactoring.h" #include "clang/Tooling/Refactoring/AtomicChange.h" -#include "clang/Tooling/Refactoring/RefactoringOptions.h" #include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h" #include "llvm/Support/Error.h" @@ -46,19 +45,12 @@ private: bool PrintLocations; }; -class NewNameOption : public RequiredRefactoringOption { -public: - StringRef getName() const override { return "new-name"; } - StringRef getDescription() const override { - return "The new name to change the symbol to"; - } -}; - /// Returns source replacements that correspond to the rename of the given /// symbol occurrences. llvm::Expected> createRenameReplacements(const SymbolOccurrences &Occurrences, - const SourceManager &SM, const SymbolName &NewName); + const SourceManager &SM, + ArrayRef NewNameStrings); /// Rename all symbols identified by the given USRs. class QualifiedRenamingAction { diff --git a/lib/Tooling/Refactoring/Rename/RenamingAction.cpp b/lib/Tooling/Refactoring/Rename/RenamingAction.cpp index 547d7bb6bc..fe3067f825 100644 --- a/lib/Tooling/Refactoring/Rename/RenamingAction.cpp +++ b/lib/Tooling/Refactoring/Rename/RenamingAction.cpp @@ -23,8 +23,6 @@ #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/SymbolName.h" #include "clang/Tooling/Refactoring/Rename/USRFinder.h" #include "clang/Tooling/Refactoring/Rename/USRFindingAction.h" #include "clang/Tooling/Refactoring/Rename/USRLocFinder.h" @@ -77,8 +75,7 @@ private: class RenameOccurrences final : public SourceChangeRefactoringRule { public: - RenameOccurrences(const NamedDecl *ND, std::string NewName) - : Finder(ND), NewName(NewName) {} + RenameOccurrences(const NamedDecl *ND) : Finder(ND) {} Expected createSourceReplacements(RefactoringRuleContext &Context) { @@ -86,15 +83,15 @@ public: Finder.findSymbolOccurrences(Context); if (!Occurrences) return Occurrences.takeError(); - // FIXME: Verify that the new name is valid. - SymbolName Name(NewName); + // FIXME: This is a temporary workaround that's needed until the refactoring + // options are implemented. + StringRef NewName = "Bar"; return createRenameReplacements( - *Occurrences, Context.getASTContext().getSourceManager(), Name); + *Occurrences, Context.getASTContext().getSourceManager(), NewName); } private: OccurrenceFinder Finder; - std::string NewName; }; class LocalRename final : public RefactoringAction { @@ -110,7 +107,7 @@ public: RefactoringActionRules createActionRules() const override { RefactoringActionRules Rules; Rules.push_back(createRefactoringActionRule( - SymbolSelectionRequirement(), OptionRequirement())); + SymbolSelectionRequirement())); return Rules; } }; @@ -123,18 +120,19 @@ std::unique_ptr createLocalRenameAction() { Expected> createRenameReplacements(const SymbolOccurrences &Occurrences, - const SourceManager &SM, const SymbolName &NewName) { + const SourceManager &SM, + ArrayRef NewNameStrings) { // FIXME: A true local rename can use just one AtomicChange. std::vector Changes; for (const auto &Occurrence : Occurrences) { ArrayRef Ranges = Occurrence.getNameRanges(); - assert(NewName.getNamePieces().size() == Ranges.size() && + assert(NewNameStrings.size() == Ranges.size() && "Mismatching number of ranges and name pieces"); AtomicChange Change(SM, Ranges[0].getBegin()); for (const auto &Range : llvm::enumerate(Ranges)) { auto Error = Change.replace(SM, CharSourceRange::getCharRange(Range.value()), - NewName.getNamePieces()[Range.index()]); + NewNameStrings[Range.index()]); if (Error) return std::move(Error); } @@ -198,7 +196,7 @@ public: } // FIXME: Support multi-piece names. // FIXME: better error handling (propagate error out). - SymbolName NewNameRef(NewName); + StringRef NewNameRef = NewName; Expected> Change = createRenameReplacements(Occurrences, SourceMgr, NewNameRef); if (!Change) { diff --git a/test/Refactor/LocalRename/Field.cpp b/test/Refactor/LocalRename/Field.cpp index 83ca2e5748..830e91d200 100644 --- a/test/Refactor/LocalRename/Field.cpp +++ b/test/Refactor/LocalRename/Field.cpp @@ -1,4 +1,4 @@ -// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | FileCheck %s +// RUN: clang-refactor local-rename -selection=test:%s %s -- | FileCheck %s class Baz { int /*range=*/Foo; // CHECK: int /*range=*/Bar; diff --git a/test/Refactor/tool-test-support.c b/test/Refactor/tool-test-support.c index a20825518c..f75b2f9f19 100644 --- a/test/Refactor/tool-test-support.c +++ b/test/Refactor/tool-test-support.c @@ -1,4 +1,4 @@ -// RUN: clang-refactor local-rename -selection=test:%s -new-name=test -v %s -- | FileCheck %s +// RUN: clang-refactor local-rename -selection=test:%s -v %s -- | FileCheck %s /*range=*/int test; @@ -11,12 +11,12 @@ /*range named =+0*/int test5; // CHECK: Test selection group '': -// CHECK-NEXT: 105-105 -// CHECK-NEXT: 158-158 -// CHECK-NEXT: 197-197 +// CHECK-NEXT: 90-90 +// CHECK-NEXT: 143-143 +// CHECK-NEXT: 182-182 // CHECK-NEXT: Test selection group 'named': -// CHECK-NEXT: 132-132 -// CHECK-NEXT: 218-218 +// CHECK-NEXT: 117-117 +// CHECK-NEXT: 203-203 // The following invocations are in the default group: diff --git a/tools/clang-refactor/ClangRefactor.cpp b/tools/clang-refactor/ClangRefactor.cpp index 47e09e7050..ff13773072 100644 --- a/tools/clang-refactor/ClangRefactor.cpp +++ b/tools/clang-refactor/ClangRefactor.cpp @@ -18,7 +18,6 @@ #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" @@ -33,10 +32,10 @@ namespace cl = llvm::cl; namespace opts { -static cl::OptionCategory CommonRefactorOptions("Refactoring options"); +static cl::OptionCategory CommonRefactorOptions("Common refactoring options"); static cl::opt Verbose("v", cl::desc("Use verbose output"), - cl::cat(cl::GeneralCategory), + cl::cat(CommonRefactorOptions), cl::sub(*cl::AllSubCommands)); } // end namespace opts @@ -117,92 +116,6 @@ SourceSelectionArgument::fromString(StringRef Value) { return nullptr; } -/// A container that stores the command-line options used by a single -/// refactoring option. -class RefactoringActionCommandLineOptions { -public: - void addStringOption(const RefactoringOption &Option, - std::unique_ptr> CLOption) { - StringOptions[&Option] = std::move(CLOption); - } - - const cl::opt & - getStringOption(const RefactoringOption &Opt) const { - auto It = StringOptions.find(&Opt); - return *It->second; - } - -private: - llvm::DenseMap>> - StringOptions; -}; - -/// Passes the command-line option values to the options used by a single -/// refactoring action rule. -class CommandLineRefactoringOptionVisitor final - : public RefactoringOptionVisitor { -public: - CommandLineRefactoringOptionVisitor( - const RefactoringActionCommandLineOptions &Options) - : Options(Options) {} - - void visit(const RefactoringOption &Opt, - Optional &Value) override { - const cl::opt &CLOpt = Options.getStringOption(Opt); - if (!CLOpt.getValue().empty()) { - Value = CLOpt.getValue(); - return; - } - Value = None; - if (Opt.isRequired()) - MissingRequiredOptions.push_back(&Opt); - } - - ArrayRef getMissingRequiredOptions() const { - return MissingRequiredOptions; - } - -private: - llvm::SmallVector MissingRequiredOptions; - const RefactoringActionCommandLineOptions &Options; -}; - -/// Creates the refactoring options used by all the rules in a single -/// refactoring action. -class CommandLineRefactoringOptionCreator final - : public RefactoringOptionVisitor { -public: - CommandLineRefactoringOptionCreator( - cl::OptionCategory &Category, cl::SubCommand &Subcommand, - RefactoringActionCommandLineOptions &Options) - : Category(Category), Subcommand(Subcommand), Options(Options) {} - - void visit(const RefactoringOption &Opt, Optional &) override { - if (Visited.insert(&Opt).second) - Options.addStringOption(Opt, create(Opt)); - } - -private: - template - std::unique_ptr> create(const RefactoringOption &Opt) { - 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 &Category; - cl::SubCommand &Subcommand; - RefactoringActionCommandLineOptions &Options; -}; - /// A subcommand that corresponds to individual refactoring action. class RefactoringActionSubcommand : public cl::SubCommand { public: @@ -225,12 +138,6 @@ public: "::)"), cl::cat(Category), cl::sub(*this)); } - // Create the refactoring options. - for (const auto &Rule : this->ActionRules) { - CommandLineRefactoringOptionCreator OptionCreator(Category, *this, - Options); - Rule->visitRefactoringOptions(OptionCreator); - } } ~RefactoringActionSubcommand() { unregisterSubCommand(); } @@ -253,17 +160,11 @@ public: assert(Selection && "selection not supported!"); return ParsedSelection.get(); } - - const RefactoringActionCommandLineOptions &getOptions() const { - return Options; - } - private: std::unique_ptr Action; RefactoringActionRules ActionRules; std::unique_ptr> Selection; std::unique_ptr ParsedSelection; - RefactoringActionCommandLineOptions Options; }; class ClangRefactorConsumer : public RefactoringResultConsumer { @@ -361,22 +262,14 @@ public: bool HasSelection = false; for (const auto &Rule : Subcommand.getActionRules()) { - bool SelectionMatches = true; if (Rule->hasSelectionRequirement()) { HasSelection = true; - if (!Subcommand.getSelection()) { + if (Subcommand.getSelection()) + MatchingRules.push_back(Rule.get()); + else MissingOptions.insert("selection"); - SelectionMatches = false; - } - } - CommandLineRefactoringOptionVisitor Visitor(Subcommand.getOptions()); - Rule->visitRefactoringOptions(Visitor); - if (SelectionMatches && Visitor.getMissingRequiredOptions().empty()) { - MatchingRules.push_back(Rule.get()); - continue; } - for (const RefactoringOption *Opt : Visitor.getMissingRequiredOptions()) - MissingOptions.insert(Opt->getName()); + // FIXME (Alex L): Support custom options. } if (MatchingRules.empty()) { llvm::errs() << "error: '" << Subcommand.getName() @@ -433,7 +326,7 @@ int main(int argc, const char **argv) { ClangRefactorTool Tool; CommonOptionsParser Options( - argc, argv, cl::GeneralCategory, cl::ZeroOrMore, + argc, argv, opts::CommonRefactorOptions, cl::ZeroOrMore, "Clang-based refactoring tool for C, C++ and Objective-C"); // Figure out which action is specified by the user. The user must specify