Author: Christian Kandeler Date: 2024-06-03T14:48:53+02:00 New Revision: 955c2237629ae252bed44177f0b65a8805051add
URL: https://github.com/llvm/llvm-project/commit/955c2237629ae252bed44177f0b65a8805051add DIFF: https://github.com/llvm/llvm-project/commit/955c2237629ae252bed44177f0b65a8805051add.diff LOG: [clangd] Allow "move function body out-of-line" in non-header files (#69704) Moving the body of member functions out-of-line makes sense for classes defined in implementation files too. Added: Modified: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp index fef827a801c33..f43f2417df8fc 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp @@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind, // looked up in the context containing the function/method. // FIXME: Drop attributes in function signature. llvm::Expected<std::string> -getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace, +getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext, const syntax::TokenBuffer &TokBuf, const HeuristicResolver *Resolver) { auto &AST = FD->getASTContext(); auto &SM = AST.getSourceManager(); - auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext()); - if (!TargetContext) - return error("define outline: couldn't find a context for target"); llvm::Error Errors = llvm::Error::success(); tooling::Replacements DeclarationCleanups; @@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace, } const NamedDecl *ND = Ref.Targets.front(); const std::string Qualifier = - getQualification(AST, *TargetContext, + getQualification(AST, TargetContext, SM.getLocForStartOfFile(SM.getMainFileID()), ND); if (auto Err = DeclarationCleanups.add( tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) @@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace, if (const auto *Destructor = llvm::dyn_cast<CXXDestructorDecl>(FD)) { if (auto Err = DeclarationCleanups.add(tooling::Replacement( SM, Destructor->getLocation(), 0, - getQualification(AST, *TargetContext, + getQualification(AST, TargetContext, SM.getLocForStartOfFile(SM.getMainFileID()), Destructor)))) Errors = llvm::joinErrors(std::move(Errors), std::move(Err)); @@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace, } struct InsertionPoint { - std::string EnclosingNamespace; + const DeclContext *EnclosingNamespace = nullptr; size_t Offset; }; -// Returns the most natural insertion point for \p QualifiedName in \p Contents. -// This currently cares about only the namespace proximity, but in feature it -// should also try to follow ordering of declarations. For example, if decls -// come in order `foo, bar, baz` then this function should return some point -// between foo and baz for inserting bar. -llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents, - llvm::StringRef QualifiedName, - const LangOptions &LangOpts) { - auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts); - - assert(!Region.EligiblePoints.empty()); - // FIXME: This selection can be made smarter by looking at the definition - // locations for adjacent decls to Source. Unfortunately pseudo parsing in - // getEligibleRegions only knows about namespace begin/end events so we - // can't match function start/end positions yet. - auto Offset = positionToOffset(Contents, Region.EligiblePoints.back()); - if (!Offset) - return Offset.takeError(); - return InsertionPoint{Region.EnclosingNamespace, *Offset}; -} // Returns the range that should be deleted from declaration, which always // contains function body. In addition to that it might contain constructor @@ -409,14 +386,9 @@ class DefineOutline : public Tweak { } bool prepare(const Selection &Sel) override { - // Bail out if we are not in a header file. - // FIXME: We might want to consider moving method definitions below class - // definition even if we are inside a source file. - if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor), - Sel.AST->getLangOpts())) - return false; - + SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts()); Source = getSelectedFunction(Sel.ASTSelection.commonAncestor()); + // Bail out if the selection is not a in-line function definition. if (!Source || !Source->doesThisDeclarationHaveABody() || Source->isOutOfLine()) @@ -429,19 +401,24 @@ class DefineOutline : public Tweak { if (Source->getTemplateSpecializationInfo()) return false; - if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source)) { - // Bail out in templated classes, as it is hard to spell the class name, - // i.e if the template parameter is unnamed. - if (MD->getParent()->isTemplated()) - return false; - - // The refactoring is meaningless for unnamed classes and definitions - // within unnamed namespaces. - for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) { - if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) { - if (ND->getDeclName().isEmpty()) - return false; - } + auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source); + if (!MD) { + // Can't outline free-standing functions in the same file. + return !SameFile; + } + + // Bail out in templated classes, as it is hard to spell the class name, + // i.e if the template parameter is unnamed. + if (MD->getParent()->isTemplated()) + return false; + + // The refactoring is meaningless for unnamed classes and namespaces, + // unless we're outlining in the same file + for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) { + if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) { + if (ND->getDeclName().isEmpty() && + (!SameFile || !llvm::dyn_cast<NamespaceDecl>(ND))) + return false; } } @@ -453,8 +430,8 @@ class DefineOutline : public Tweak { Expected<Effect> apply(const Selection &Sel) override { const SourceManager &SM = Sel.AST->getSourceManager(); - auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel); - + auto CCFile = SameFile ? Sel.AST->tuPath().str() + : getSourceFile(Sel.AST->tuPath(), Sel); if (!CCFile) return error("Couldn't find a suitable implementation file."); assert(Sel.FS && "FS Must be set in apply"); @@ -464,8 +441,7 @@ class DefineOutline : public Tweak { if (!Buffer) return llvm::errorCodeToError(Buffer.getError()); auto Contents = Buffer->get()->getBuffer(); - auto InsertionPoint = getInsertionPoint( - Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts()); + auto InsertionPoint = getInsertionPoint(Contents, Sel); if (!InsertionPoint) return InsertionPoint.takeError(); @@ -499,17 +475,77 @@ class DefineOutline : public Tweak { HeaderUpdates = HeaderUpdates.merge(*DelInline); } - auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates); - if (!HeaderFE) - return HeaderFE.takeError(); - - Effect->ApplyEdits.try_emplace(HeaderFE->first, - std::move(HeaderFE->second)); + if (SameFile) { + tooling::Replacements &R = Effect->ApplyEdits[*CCFile].Replacements; + R = R.merge(HeaderUpdates); + } else { + auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates); + if (!HeaderFE) + return HeaderFE.takeError(); + Effect->ApplyEdits.try_emplace(HeaderFE->first, + std::move(HeaderFE->second)); + } return std::move(*Effect); } + // Returns the most natural insertion point for \p QualifiedName in \p + // Contents. This currently cares about only the namespace proximity, but in + // feature it should also try to follow ordering of declarations. For example, + // if decls come in order `foo, bar, baz` then this function should return + // some point between foo and baz for inserting bar. + // FIXME: The selection can be made smarter by looking at the definition + // locations for adjacent decls to Source. Unfortunately pseudo parsing in + // getEligibleRegions only knows about namespace begin/end events so we + // can't match function start/end positions yet. + llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents, + const Selection &Sel) { + // If the definition goes to the same file and there is a namespace, + // we should (and, in the case of anonymous namespaces, need to) + // put the definition into the original namespace block. + if (SameFile) { + auto *Klass = Source->getDeclContext()->getOuterLexicalRecordContext(); + if (!Klass) + return error("moving to same file not supported for free functions"); + const SourceLocation EndLoc = Klass->getBraceRange().getEnd(); + const auto &TokBuf = Sel.AST->getTokens(); + auto Tokens = TokBuf.expandedTokens(); + auto It = llvm::lower_bound( + Tokens, EndLoc, [](const syntax::Token &Tok, SourceLocation EndLoc) { + return Tok.location() < EndLoc; + }); + while (It != Tokens.end()) { + if (It->kind() != tok::semi) { + ++It; + continue; + } + unsigned Offset = Sel.AST->getSourceManager() + .getDecomposedLoc(It->endLocation()) + .second; + return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset}; + } + return error( + "failed to determine insertion location: no end of class found"); + } + + auto Region = getEligiblePoints( + Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts()); + + assert(!Region.EligiblePoints.empty()); + auto Offset = positionToOffset(Contents, Region.EligiblePoints.back()); + if (!Offset) + return Offset.takeError(); + + auto TargetContext = + findContextForNS(Region.EnclosingNamespace, Source->getDeclContext()); + if (!TargetContext) + return error("define outline: couldn't find a context for target"); + + return InsertionPoint{*TargetContext, *Offset}; + } + private: const FunctionDecl *Source = nullptr; + bool SameFile = false; }; REGISTER_TWEAK(DefineOutline) diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp index d1e60b070f20e..906ff33db8734 100644 --- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp @@ -19,12 +19,47 @@ TWEAK_TEST(DefineOutline); TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) { FileName = "Test.cpp"; - // Not available unless in a header file. + // Not available for free function unless in a header file. EXPECT_UNAVAILABLE(R"cpp( [[void [[f^o^o]]() [[{ return; }]]]])cpp"); + // Available in soure file. + EXPECT_AVAILABLE(R"cpp( + struct Foo { + void f^oo() {} + }; + )cpp"); + + // Available within named namespace in source file. + EXPECT_AVAILABLE(R"cpp( + namespace N { + struct Foo { + void f^oo() {} + }; + } // namespace N + )cpp"); + + // Available within anonymous namespace in source file. + EXPECT_AVAILABLE(R"cpp( + namespace { + struct Foo { + void f^oo() {} + }; + } // namespace + )cpp"); + + // Not available for out-of-line method. + EXPECT_UNAVAILABLE(R"cpp( + class Bar { + void baz(); + }; + + [[void [[Bar::[[b^a^z]]]]() [[{ + return; + }]]]])cpp"); + FileName = "Test.hpp"; // Not available unless function name or fully body is selected. EXPECT_UNAVAILABLE(R"cpp( @@ -100,7 +135,7 @@ TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) { }; )cpp"); - // Not available on definitions within unnamed namespaces + // Not available on definitions in header file within unnamed namespaces EXPECT_UNAVAILABLE(R"cpp( namespace { struct Foo { @@ -349,6 +384,40 @@ TEST_F(DefineOutlineTest, ApplyTest) { } } +TEST_F(DefineOutlineTest, InCppFile) { + FileName = "Test.cpp"; + + struct { + llvm::StringRef Test; + llvm::StringRef ExpectedSource; + } Cases[] = { + { + R"cpp( + namespace foo { + namespace { + struct Foo { void ba^r() {} }; + struct Bar { void foo(); }; + void Bar::foo() {} + } + } + )cpp", + R"cpp( + namespace foo { + namespace { + struct Foo { void bar() ; };void Foo::bar() {} + struct Bar { void foo(); }; + void Bar::foo() {} + } + } + )cpp"}, + }; + + for (const auto &Case : Cases) { + SCOPED_TRACE(Case.Test); + EXPECT_EQ(apply(Case.Test, nullptr), Case.ExpectedSource); + } +} + TEST_F(DefineOutlineTest, HandleMacros) { llvm::StringMap<std::string> EditedFiles; ExtraFiles["Test.cpp"] = ""; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
