Index: clangd/ClangdServer.cpp =================================================================== --- clangd/ClangdServer.cpp +++ clangd/ClangdServer.cpp @@ -329,23 +329,28 @@ "Rename", File, Bind(Action, File.str(), NewName.str(), std::move(CB))); } +static llvm::Expected +tweakSelection(const Range &Sel, const InputsAndAST &AST) { + auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start); + if (!Begin) + return Begin.takeError(); + auto End = positionToOffset(AST.Inputs.Contents, Sel.end); + if (!End) + return End.takeError(); + return Tweak::Selection(AST.AST, *Begin, *End); +} + void ClangdServer::enumerateTweaks(PathRef File, Range Sel, Callback> CB) { auto Action = [Sel](decltype(CB) CB, std::string File, Expected InpAST) { if (!InpAST) return CB(InpAST.takeError()); - - auto &AST = InpAST->AST; - auto CursorLoc = sourceLocationInMainFile( - AST.getASTContext().getSourceManager(), Sel.start); - if (!CursorLoc) - return CB(CursorLoc.takeError()); - Tweak::Selection Inputs = {InpAST->Inputs.Contents, InpAST->AST, - *CursorLoc}; - + auto Selection = tweakSelection(Sel, *InpAST); + if (!Selection) + return CB(Selection.takeError()); std::vector Res; - for (auto &T : prepareTweaks(Inputs)) + for (auto &T : prepareTweaks(*Selection)) Res.push_back({T->id(), T->title()}); CB(std::move(Res)); }; @@ -360,20 +365,14 @@ Expected InpAST) { if (!InpAST) return CB(InpAST.takeError()); - - auto &AST = InpAST->AST; - auto CursorLoc = sourceLocationInMainFile( - AST.getASTContext().getSourceManager(), Sel.start); - if (!CursorLoc) - return CB(CursorLoc.takeError()); - Tweak::Selection Inputs = {InpAST->Inputs.Contents, InpAST->AST, - *CursorLoc}; - - auto A = prepareTweak(TweakID, Inputs); + auto Selection = tweakSelection(Sel, *InpAST); + if (!Selection) + return CB(Selection.takeError()); + auto A = prepareTweak(TweakID, *Selection); if (!A) return CB(A.takeError()); // FIXME: run formatter on top of resulting replacements. - return CB((*A)->apply(Inputs)); + return CB((*A)->apply(*Selection)); }; WorkScheduler.runWithAST( "ApplyTweak", File, Index: clangd/refactor/Tweak.h =================================================================== --- clangd/refactor/Tweak.h +++ clangd/refactor/Tweak.h @@ -21,6 +21,7 @@ #include "ClangdUnit.h" #include "Protocol.h" +#include "Selection.h" #include "clang/Tooling/Core/Replacement.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/StringRef.h" @@ -39,16 +40,16 @@ public: /// Input to prepare and apply tweaks. struct Selection { + Selection(ParsedAST &AST, unsigned RangeBegin, unsigned RangeEnd); /// The text of the active document. llvm::StringRef Code; /// Parsed AST of the active file. ParsedAST &AST; /// A location of the cursor in the editor. SourceLocation Cursor; - // FIXME: add selection when there are checks relying on it. + // The AST nodes that were selected. + SelectionTree ASTSelection; // FIXME: provide a way to get sources and ASTs for other files. - // FIXME: cache some commonly required information (e.g. AST nodes under - // cursor) to avoid redundant AST visit in every action. }; virtual ~Tweak() = default; /// A unique id of the action, it is always equal to the name of the class Index: clangd/refactor/Tweak.cpp =================================================================== --- clangd/refactor/Tweak.cpp +++ clangd/refactor/Tweak.cpp @@ -38,6 +38,14 @@ } } // namespace +Tweak::Selection::Selection(ParsedAST &AST, unsigned RangeBegin, + unsigned RangeEnd) + : AST(AST), ASTSelection(AST.getASTContext(), RangeBegin, RangeEnd) { + auto &SM = AST.getASTContext().getSourceManager(); + Code = SM.getBufferData(SM.getMainFileID()); + Cursor = SM.getComposedLoc(SM.getMainFileID(), RangeBegin); +} + std::vector> prepareTweaks(const Tweak::Selection &S) { validateRegistry(); Index: clangd/refactor/tweaks/SwapIfBranches.cpp =================================================================== --- clangd/refactor/tweaks/SwapIfBranches.cpp +++ clangd/refactor/tweaks/SwapIfBranches.cpp @@ -41,58 +41,23 @@ std::string title() const override; private: - IfStmt *If = nullptr; + const IfStmt *If = nullptr; }; REGISTER_TWEAK(SwapIfBranches); -class FindIfUnderCursor : public RecursiveASTVisitor { -public: - FindIfUnderCursor(ASTContext &Ctx, SourceLocation CursorLoc, IfStmt *&Result) - : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {} - - bool VisitIfStmt(IfStmt *If) { - // Check if the cursor is in the range of 'if (cond)'. - // FIXME: this does not contain the closing paren, add it too. - auto R = toHalfOpenFileRange( - Ctx.getSourceManager(), Ctx.getLangOpts(), - SourceRange(If->getIfLoc(), If->getCond()->getEndLoc().isValid() - ? If->getCond()->getEndLoc() - : If->getIfLoc())); - if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) { - Result = If; - return false; - } - // Check the range of 'else'. - R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(), - SourceRange(If->getElseLoc())); - if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) { - Result = If; +bool SwapIfBranches::prepare(const Selection &Inputs) { + for (const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor(); + N && !If; N = N->Parent) { + // Stop once we hit a block, e.g. a lambda in the if condition. + if (dyn_cast_or_null(N->ASTNode.get())) return false; - } - - return true; + If = dyn_cast_or_null(N->ASTNode.get()); } - -private: - ASTContext &Ctx; - SourceLocation CursorLoc; - IfStmt *&Result; -}; -} // namespace - -bool SwapIfBranches::prepare(const Selection &Inputs) { - auto &Ctx = Inputs.AST.getASTContext(); - FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx); - if (!If) - return false; - // avoid dealing with single-statement brances, they require careful handling // to avoid changing semantics of the code (i.e. dangling else). - if (!If->getThen() || !llvm::isa(If->getThen()) || - !If->getElse() || !llvm::isa(If->getElse())) - return false; - return true; + return If && dyn_cast_or_null(If->getThen()) && + llvm::dyn_cast_or_null(If->getElse()); } Expected SwapIfBranches::apply(const Selection &Inputs) { @@ -128,5 +93,7 @@ } std::string SwapIfBranches::title() const { return "Swap if branches"; } + +} // namespace } // namespace clangd } // namespace clang Index: unittests/clangd/TweakTests.cpp =================================================================== --- unittests/clangd/TweakTests.cpp +++ unittests/clangd/TweakTests.cpp @@ -52,9 +52,9 @@ ParsedAST AST = TU.build(); auto CheckOver = [&](Range Selection) { - auto CursorLoc = llvm::cantFail(sourceLocationInMainFile( - AST.getASTContext().getSourceManager(), Selection.start)); - auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc}); + unsigned Begin = cantFail(positionToOffset(Code.code(), Selection.start)), + End = cantFail(positionToOffset(Code.code(), Selection.end)); + auto T = prepareTweak(ID, Tweak::Selection(AST, Begin, End)); if (Available) EXPECT_THAT_EXPECTED(T, Succeeded()) << "code is " << markRange(Code.code(), Selection); @@ -92,9 +92,9 @@ TU.Code = Code.code(); ParsedAST AST = TU.build(); - auto CursorLoc = llvm::cantFail(sourceLocationInMainFile( - AST.getASTContext().getSourceManager(), SelectionRng.start)); - Tweak::Selection S = {Code.code(), AST, CursorLoc}; + unsigned Begin = cantFail(positionToOffset(Code.code(), SelectionRng.start)), + End = cantFail(positionToOffset(Code.code(), SelectionRng.end)); + Tweak::Selection S(AST, Begin, End); auto T = prepareTweak(ID, S); if (!T) @@ -149,6 +149,37 @@ } )cpp"; checkTransform(ID, Input, Output); + + // Available in subexpressions of the condition. + checkAvailable(ID, R"cpp( + void test() { + if(2 + [[2]] + 2) { return 2 + 2 + 2; } else { continue; } + } + )cpp"); + // But not as part of the branches. + checkNotAvailable(ID, R"cpp( + void test() { + if(2 + 2 + 2) { return 2 + [[2]] + 2; } else { continue; } + } + )cpp"); + // Range covers the "else" token, so available. + checkAvailable(ID, R"cpp( + void test() { + if(2 + 2 + 2) { return 2 + [[2 + 2; } else { continue;]] } + } + )cpp"); + // Not available in compound statements in condition. + checkNotAvailable(ID, R"cpp( + void test() { + if([]{return [[true]];}()) { return 2 + 2 + 2; } else { continue; } + } + )cpp"); + // Not available if both sides aren't braced. + checkNotAvailable(ID, R"cpp( + void test() { + ^if (1) return; else { return; } + } + )cpp"); } } // namespace