Author: Adam Czachorowski Date: 2020-12-08T15:55:32+01:00 New Revision: f6b205dae16392382324fbca676ef6afe3920642
URL: https://github.com/llvm/llvm-project/commit/f6b205dae16392382324fbca676ef6afe3920642 DIFF: https://github.com/llvm/llvm-project/commit/f6b205dae16392382324fbca676ef6afe3920642.diff LOG: [clangd] ExtractFunction: disable on regions that sometimes, but not always return. apply() will fail in those cases, so it's better to detect it in prepare() already and hide code action from the user. This was especially annoying on code bases that use a lot of RETURN_IF_ERROR-like macros. Differential Revision: https://reviews.llvm.org/D92408 Added: Modified: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp clang-tools-extra/clangd/unittests/TweakTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 18fe7bf39160..8eec42876d6b 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -708,6 +708,27 @@ tooling::Replacement createFunctionDefinition(const NewFunction &ExtractedFunc, return tooling::Replacement(SM, ExtractedFunc.InsertionPoint, 0, FunctionDef); } +// Returns true if ExtZone contains any ReturnStmts. +bool hasReturnStmt(const ExtractionZone &ExtZone) { + class ReturnStmtVisitor + : public clang::RecursiveASTVisitor<ReturnStmtVisitor> { + public: + bool VisitReturnStmt(ReturnStmt *Return) { + Found = true; + return false; // We found the answer, abort the scan. + } + bool Found = false; + }; + + ReturnStmtVisitor V; + for (const Stmt *RootStmt : ExtZone.RootStmts) { + V.TraverseStmt(const_cast<Stmt *>(RootStmt)); + if (V.Found) + break; + } + return V.Found; +} + bool ExtractFunction::prepare(const Selection &Inputs) { const LangOptions &LangOpts = Inputs.AST->getLangOpts(); if (!LangOpts.CPlusPlus) @@ -715,8 +736,12 @@ bool ExtractFunction::prepare(const Selection &Inputs) { const Node *CommonAnc = Inputs.ASTSelection.commonAncestor(); const SourceManager &SM = Inputs.AST->getSourceManager(); auto MaybeExtZone = findExtractionZone(CommonAnc, SM, LangOpts); + if (!MaybeExtZone || + (hasReturnStmt(*MaybeExtZone) && !alwaysReturns(*MaybeExtZone))) + return false; + // FIXME: Get rid of this check once we support hoisting. - if (!MaybeExtZone || MaybeExtZone->requiresHoisting(SM)) + if (MaybeExtZone->requiresHoisting(SM)) return false; ExtZone = std::move(*MaybeExtZone); diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp index 07f061b3f2e3..85edd92d27da 100644 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -607,7 +607,11 @@ TEST_F(ExtractFunctionTest, FunctionTest) { // Extract certain return EXPECT_THAT(apply(" if(true) [[{ return; }]] "), HasSubstr("extracted")); // Don't extract uncertain return - EXPECT_THAT(apply(" if(true) [[if (false) return;]] "), StartsWith("fail")); + EXPECT_THAT(apply(" if(true) [[if (false) return;]] "), + StartsWith("unavailable")); + EXPECT_THAT( + apply("#define RETURN_IF_ERROR(x) if (x) return\nRETU^RN_IF_ERROR(4);"), + StartsWith("unavailable")); FileName = "a.c"; EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("unavailable")); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
