From 8a32f3b7a0f67bedbeb8df9e1bfdbb2aa846adaf Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Fri, 1 Feb 2019 05:41:50 +0000 Subject: [PATCH] [clangd] Fix crash in applyTweak, remove TweakID alias. Strings are complicated, giving them opaque names makes us forget they're complicated. llvm-svn: 352837 --- clang-tools-extra/clangd/ClangdServer.cpp | 13 +++++++------ clang-tools-extra/clangd/ClangdServer.h | 4 ++-- clang-tools-extra/clangd/refactor/Tweak.cpp | 2 +- clang-tools-extra/clangd/refactor/Tweak.h | 13 ++++--------- clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp | 2 +- clang-tools-extra/unittests/clangd/TweakTests.cpp | 10 ++++------ 6 files changed, 19 insertions(+), 25 deletions(-) diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 7221b38..6857b42 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -354,10 +354,10 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel, Bind(Action, std::move(CB), File.str())); } -void ClangdServer::applyTweak(PathRef File, Range Sel, TweakID ID, +void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID, Callback CB) { - auto Action = [ID, Sel](decltype(CB) CB, std::string File, - Expected InpAST) { + auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID, + Expected InpAST) { if (!InpAST) return CB(InpAST.takeError()); @@ -369,14 +369,15 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, TweakID ID, Tweak::Selection Inputs = {InpAST->Inputs.Contents, InpAST->AST, *CursorLoc}; - auto A = prepareTweak(ID, Inputs); + auto A = prepareTweak(TweakID, Inputs); if (!A) return CB(A.takeError()); // FIXME: run formatter on top of resulting replacements. return CB((*A)->apply(Inputs)); }; - WorkScheduler.runWithAST("ApplyTweak", File, - Bind(Action, std::move(CB), File.str())); + WorkScheduler.runWithAST( + "ApplyTweak", File, + Bind(Action, std::move(CB), File.str(), TweakID.str())); } void ClangdServer::dumpAST(PathRef File, diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index cc9f713..f53718f 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -214,7 +214,7 @@ public: Callback> CB); struct TweakRef { - TweakID ID; /// ID to pass for applyTweak. + std::string ID; /// ID to pass for applyTweak. std::string Title; /// A single-line message to show in the UI. }; /// Enumerate the code tweaks available to the user at a specified point. @@ -222,7 +222,7 @@ public: Callback> CB); /// Apply the code tweak with a specified \p ID. - void applyTweak(PathRef File, Range Sel, TweakID ID, + void applyTweak(PathRef File, Range Sel, StringRef ID, Callback CB); /// Only for testing purposes. diff --git a/clang-tools-extra/clangd/refactor/Tweak.cpp b/clang-tools-extra/clangd/refactor/Tweak.cpp index de6020c..02a2244 100644 --- a/clang-tools-extra/clangd/refactor/Tweak.cpp +++ b/clang-tools-extra/clangd/refactor/Tweak.cpp @@ -55,7 +55,7 @@ std::vector> prepareTweaks(const Tweak::Selection &S) { return Available; } -llvm::Expected> prepareTweak(TweakID ID, +llvm::Expected> prepareTweak(StringRef ID, const Tweak::Selection &S) { auto It = llvm::find_if( TweakRegistry::entries(), diff --git a/clang-tools-extra/clangd/refactor/Tweak.h b/clang-tools-extra/clangd/refactor/Tweak.h index df00bf7..abd4995 100644 --- a/clang-tools-extra/clangd/refactor/Tweak.h +++ b/clang-tools-extra/clangd/refactor/Tweak.h @@ -27,14 +27,11 @@ namespace clang { namespace clangd { -using TweakID = llvm::StringRef; - /// An interface base for small context-sensitive refactoring actions. /// To implement a new tweak use the following pattern in a .cpp file: /// class MyTweak : public Tweak { /// public: -/// TweakID id() const override final; // definition provided by -/// // REGISTER_TWEAK. +/// const char* id() const override final; // defined by REGISTER_TWEAK. /// // implement other methods here. /// }; /// REGISTER_TWEAK(MyTweak); @@ -57,7 +54,7 @@ public: /// A unique id of the action, it is always equal to the name of the class /// defining the Tweak. Definition is provided automatically by /// REGISTER_TWEAK. - virtual TweakID id() const = 0; + virtual const char *id() const = 0; /// Run the first stage of the action. The non-None result indicates that the /// action is available and should be shown to the user. Returns None if the /// action is not available. @@ -78,9 +75,7 @@ public: #define REGISTER_TWEAK(Subclass) \ ::llvm::Registry<::clang::clangd::Tweak>::Add \ TweakRegistrationFor##Subclass(#Subclass, /*Description=*/""); \ - ::clang::clangd::TweakID Subclass::id() const { \ - return llvm::StringLiteral(#Subclass); \ - } + const char *Subclass::id() const { return #Subclass; } /// Calls prepare() on all tweaks, returning those that can run on the /// selection. @@ -89,7 +84,7 @@ std::vector> prepareTweaks(const Tweak::Selection &S); // Calls prepare() on the tweak with a given ID. // If prepare() returns false, returns an error. // If prepare() returns true, returns the corresponding tweak. -llvm::Expected> prepareTweak(TweakID ID, +llvm::Expected> prepareTweak(StringRef TweakID, const Tweak::Selection &S); } // namespace clangd diff --git a/clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp b/clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp index 4de498f..2e0f33a 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp @@ -34,7 +34,7 @@ namespace { /// if (foo) { continue; } else { return 10; } class SwapIfBranches : public Tweak { public: - TweakID id() const override final; + const char *id() const override final; bool prepare(const Selection &Inputs) override; Expected apply(const Selection &Inputs) override; diff --git a/clang-tools-extra/unittests/clangd/TweakTests.cpp b/clang-tools-extra/unittests/clangd/TweakTests.cpp index 8306b48..905f06b 100644 --- a/clang-tools-extra/unittests/clangd/TweakTests.cpp +++ b/clang-tools-extra/unittests/clangd/TweakTests.cpp @@ -24,8 +24,6 @@ using llvm::Failed; using llvm::HasValue; using llvm::Succeeded; -using ::testing::IsEmpty; -using ::testing::Not; namespace clang { namespace clangd { @@ -43,7 +41,7 @@ std::string markRange(llvm::StringRef Code, Range R) { .str(); } -void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) { +void checkAvailable(StringRef ID, llvm::StringRef Input, bool Available) { Annotations Code(Input); ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size()) << "no points of interest specified"; @@ -71,15 +69,15 @@ void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) { } /// Checks action is available at every point and range marked in \p Input. -void checkAvailable(TweakID ID, llvm::StringRef Input) { +void checkAvailable(StringRef ID, llvm::StringRef Input) { return checkAvailable(ID, Input, /*Available=*/true); } /// Same as checkAvailable, but checks the action is not available. -void checkNotAvailable(TweakID ID, llvm::StringRef Input) { +void checkNotAvailable(StringRef ID, llvm::StringRef Input) { return checkAvailable(ID, Input, /*Available=*/false); } -llvm::Expected apply(TweakID ID, llvm::StringRef Input) { +llvm::Expected apply(StringRef ID, llvm::StringRef Input) { Annotations Code(Input); Range SelectionRng; if (Code.points().size() != 0) { -- 2.7.4