[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
tom-anders wrote: > We should probably rekindle the discussion on the associated bug report; I > think users (rightly) expect this feature. Agreed, [maybe a protocol extension would also be an option](https://github.com/clangd/clangd/issues/445#issuecomment-1250355799) https://github.com/llvm/llvm-project/pull/71950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
ckandeler wrote: We should probably rekindle the discussion on the associated bug report; I think users (rightly) expect this feature. https://github.com/llvm/llvm-project/pull/71950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
tom-anders wrote: > The new patch set properly considers "external" definitions (using the > frowned-upon index lookup). I've been using this patch for a few weeks now, and it works very well. Haven't encountered any issues/crashes yet. Performance also seems reasonable (working in a medium-sized project) https://github.com/llvm/llvm-project/pull/71950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
ckandeler wrote: The new patch set properly considers "external" definitions (using the frowned-upon index lookup). https://github.com/llvm/llvm-project/pull/71950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
https://github.com/ckandeler updated
https://github.com/llvm/llvm-project/pull/71950
>From 2d7d6c9b0939bc4b300c53d2d49c20b549a561dd Mon Sep 17 00:00:00 2001
From: Christian Kandeler
Date: Fri, 10 Nov 2023 16:01:18 +0100
Subject: [PATCH] [clangd] Let DefineOutline tweak create a definition from
scratch
---
.../clangd/refactor/tweaks/DefineOutline.cpp | 74 ---
.../unittests/tweaks/DefineOutlineTests.cpp | 32 ++--
2 files changed, 89 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..95d18eca8e5df71 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -12,6 +12,7 @@
#include "ParsedAST.h"
#include "Selection.h"
#include "SourceCode.h"
+#include "XRefs.h"
#include "refactor/Tweak.h"
#include "support/Logger.h"
#include "support/Path.h"
@@ -128,7 +129,16 @@ getFunctionSourceAfterReplacements(const FunctionDecl *FD,
SM.getBufferData(SM.getMainFileID()), Replacements);
if (!QualifiedFunc)
return QualifiedFunc.takeError();
- return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+ auto QualifiedFuncString =
+ QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+ if (!FD->hasBody() && !FD->isExplicitlyDefaulted()) {
+QualifiedFuncString.pop_back(); // The semicolon finishing the declaration.
+QualifiedFuncString += " {";
+if (!FD->getReturnType()->isVoidType())
+ QualifiedFuncString += " return {}; ";
+QualifiedFuncString += "}";
+ }
+ return QualifiedFuncString;
}
// Returns replacements to delete tokens with kind `Kind` in the range
@@ -348,6 +358,30 @@ llvm::Expected
getInsertionPoint(llvm::StringRef Contents,
// initializers.
SourceRange getDeletionRange(const FunctionDecl *FD,
const syntax::TokenBuffer &TokBuf) {
+ if (!FD->hasBody()) {
+if (!FD->isExplicitlyDefaulted())
+ return {};
+
+auto tokens = TokBuf.expandedTokens(FD->getSourceRange());
+for (auto It = std::rbegin(tokens); It != std::rend(tokens); ++It) {
+ if (It->kind() == tok::kw_default) {
+const auto nextIt = std::next(It);
+if (nextIt == std::rend(tokens))
+ return {};
+const auto &ExpandedEquals = *nextIt;
+if (ExpandedEquals.kind() != tok::equal)
+ return {};
+auto SpelledEquals =
+TokBuf.spelledForExpanded(llvm::ArrayRef(ExpandedEquals));
+if (!SpelledEquals)
+ return {};
+return {SpelledEquals->front().location(),
+FD->getDefaultLoc().getLocWithOffset(1)};
+ }
+}
+return {};
+ }
+
auto DeletionRange = FD->getBody()->getSourceRange();
if (auto *CD = llvm::dyn_cast(FD)) {
// AST doesn't contain the location for ":" in ctor initializers. Therefore
@@ -405,7 +439,9 @@ class DefineOutline : public Tweak {
return CodeAction::REFACTOR_KIND;
}
std::string title() const override {
-return "Move function body to out-of-line";
+if (Source->doesThisDeclarationHaveABody())
+ return "Move function body to out-of-line";
+return "Create function body out-of-line";
}
bool prepare(const Selection &Sel) override {
@@ -417,11 +453,22 @@ class DefineOutline : public Tweak {
return false;
Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
-// Bail out if the selection is not a in-line function definition.
-if (!Source || !Source->doesThisDeclarationHaveABody() ||
-Source->isOutOfLine())
+// Bail out if the selection is not a function declaration.
+if (!Source || Source->isDeleted() || Source->isOutOfLine())
return false;
+// Bail out if a definition exists somewhere else.
+if (!Source->hasBody() && !Source->isExplicitlyDefaulted() && Sel.Index) {
+ bool HasDefinition = false;
+ Sel.Index->lookup({{getSymbolID(Source)}},
+[&HasDefinition](const Symbol &S) {
+ if (S.Definition)
+HasDefinition = true;
+});
+ if (HasDefinition)
+return false;
+}
+
// Bail out if this is a function template or specialization, as their
// definitions need to be visible in all including translation units.
if (Source->getDescribedFunctionTemplate())
@@ -483,12 +530,17 @@ class DefineOutline : public Tweak {
if (!Effect)
return Effect.takeError();
-tooling::Replacements HeaderUpdates(tooling::Replacement(
-Sel.AST->getSourceManager(),
-CharSourceRange::getTokenRange(*toHalfOpenFileRange(
-SM, Sel.AST->getLangOpts(),
-getDeletionRange(Source, Sel.AST->getTokens(,
-";"));
+tooling::Replacements HeaderUpdates;
+auto DeletionRange = getDeletionRange(Source, Sel
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
ckandeler wrote: > This implements > [clangd/clangd#445](https://github.com/clangd/clangd/issues/445) Aha, so this implementation does not work, because it relies on the AST only for figuring out whether an implementation exists. And querying the index is considered too expensive during the prepare() step. That's a pity then. https://github.com/llvm/llvm-project/pull/71950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
5chmidti wrote: This implements clangd/clangd#445 https://github.com/llvm/llvm-project/pull/71950 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
llvmbot wrote:
@llvm/pr-subscribers-clangd
Author: None (ckandeler)
Changes
---
Full diff: https://github.com/llvm/llvm-project/pull/71950.diff
2 Files Affected:
- (modified) clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
(+50-11)
- (modified) clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
(+26-6)
``diff
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..9e715b7bf21175a 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -128,7 +128,16 @@ getFunctionSourceAfterReplacements(const FunctionDecl *FD,
SM.getBufferData(SM.getMainFileID()), Replacements);
if (!QualifiedFunc)
return QualifiedFunc.takeError();
- return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+ auto QualifiedFuncString =
+ QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+ if (!FD->hasBody() && !FD->isExplicitlyDefaulted()) {
+QualifiedFuncString.pop_back(); // The semicolon finishing the declaration.
+QualifiedFuncString += " {";
+if (!FD->getReturnType()->isVoidType())
+ QualifiedFuncString += " return {}; ";
+QualifiedFuncString += "}";
+ }
+ return QualifiedFuncString;
}
// Returns replacements to delete tokens with kind `Kind` in the range
@@ -348,6 +357,30 @@ llvm::Expected
getInsertionPoint(llvm::StringRef Contents,
// initializers.
SourceRange getDeletionRange(const FunctionDecl *FD,
const syntax::TokenBuffer &TokBuf) {
+ if (!FD->hasBody()) {
+if (!FD->isExplicitlyDefaulted())
+ return {};
+
+auto tokens = TokBuf.expandedTokens(FD->getSourceRange());
+for (auto It = std::rbegin(tokens); It != std::rend(tokens); ++It) {
+ if (It->kind() == tok::kw_default) {
+const auto nextIt = std::next(It);
+if (nextIt == std::rend(tokens))
+ return {};
+const auto &ExpandedEquals = *nextIt;
+if (ExpandedEquals.kind() != tok::equal)
+ return {};
+auto SpelledEquals =
+TokBuf.spelledForExpanded(llvm::ArrayRef(ExpandedEquals));
+if (!SpelledEquals)
+ return {};
+return {SpelledEquals->front().location(),
+FD->getDefaultLoc().getLocWithOffset(1)};
+ }
+}
+return {};
+ }
+
auto DeletionRange = FD->getBody()->getSourceRange();
if (auto *CD = llvm::dyn_cast(FD)) {
// AST doesn't contain the location for ":" in ctor initializers. Therefore
@@ -405,7 +438,9 @@ class DefineOutline : public Tweak {
return CodeAction::REFACTOR_KIND;
}
std::string title() const override {
-return "Move function body to out-of-line";
+if (Source->doesThisDeclarationHaveABody())
+ return "Move function body to out-of-line";
+return "Create function body out-of-line";
}
bool prepare(const Selection &Sel) override {
@@ -417,9 +452,8 @@ class DefineOutline : public Tweak {
return false;
Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
-// Bail out if the selection is not a in-line function definition.
-if (!Source || !Source->doesThisDeclarationHaveABody() ||
-Source->isOutOfLine())
+// Bail out if the selection is not a function declaration.
+if (!Source || Source->isDeleted() || Source->isOutOfLine())
return false;
// Bail out if this is a function template or specialization, as their
@@ -483,12 +517,17 @@ class DefineOutline : public Tweak {
if (!Effect)
return Effect.takeError();
-tooling::Replacements HeaderUpdates(tooling::Replacement(
-Sel.AST->getSourceManager(),
-CharSourceRange::getTokenRange(*toHalfOpenFileRange(
-SM, Sel.AST->getLangOpts(),
-getDeletionRange(Source, Sel.AST->getTokens(,
-";"));
+tooling::Replacements HeaderUpdates;
+auto DeletionRange = getDeletionRange(Source, Sel.AST->getTokens());
+if (DeletionRange.isValid()) {
+ if (auto Error = HeaderUpdates.add(tooling::Replacement(
+ Sel.AST->getSourceManager(),
+ CharSourceRange::getTokenRange(*toHalfOpenFileRange(
+ SM, Sel.AST->getLangOpts(), DeletionRange)),
+ ";"))) {
+return Error;
+ }
+}
if (Source->isInlineSpecified()) {
auto DelInline =
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index d1e60b070f20e95..dede5f7c3c25e8b 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -28,9 +28,6 @@ TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
FileName = "Test.hpp";
// Not available unless function name or fully body is selected.
[clang-tools-extra] [clangd] Let DefineOutline tweak create a definition from scratch (PR #71950)
https://github.com/ckandeler created
https://github.com/llvm/llvm-project/pull/71950
None
>From b32023796e29ad02080519f7f2cfa98307f7d1d9 Mon Sep 17 00:00:00 2001
From: Christian Kandeler
Date: Fri, 10 Nov 2023 16:01:18 +0100
Subject: [PATCH] [clangd] Let DefineOutline tweak create a definition from
scratch
---
.../clangd/refactor/tweaks/DefineOutline.cpp | 61 +++
.../unittests/tweaks/DefineOutlineTests.cpp | 32 --
2 files changed, 76 insertions(+), 17 deletions(-)
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index b84ae04072f2c19..9e715b7bf21175a 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -128,7 +128,16 @@ getFunctionSourceAfterReplacements(const FunctionDecl *FD,
SM.getBufferData(SM.getMainFileID()), Replacements);
if (!QualifiedFunc)
return QualifiedFunc.takeError();
- return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+ auto QualifiedFuncString =
+ QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
+ if (!FD->hasBody() && !FD->isExplicitlyDefaulted()) {
+QualifiedFuncString.pop_back(); // The semicolon finishing the declaration.
+QualifiedFuncString += " {";
+if (!FD->getReturnType()->isVoidType())
+ QualifiedFuncString += " return {}; ";
+QualifiedFuncString += "}";
+ }
+ return QualifiedFuncString;
}
// Returns replacements to delete tokens with kind `Kind` in the range
@@ -348,6 +357,30 @@ llvm::Expected
getInsertionPoint(llvm::StringRef Contents,
// initializers.
SourceRange getDeletionRange(const FunctionDecl *FD,
const syntax::TokenBuffer &TokBuf) {
+ if (!FD->hasBody()) {
+if (!FD->isExplicitlyDefaulted())
+ return {};
+
+auto tokens = TokBuf.expandedTokens(FD->getSourceRange());
+for (auto It = std::rbegin(tokens); It != std::rend(tokens); ++It) {
+ if (It->kind() == tok::kw_default) {
+const auto nextIt = std::next(It);
+if (nextIt == std::rend(tokens))
+ return {};
+const auto &ExpandedEquals = *nextIt;
+if (ExpandedEquals.kind() != tok::equal)
+ return {};
+auto SpelledEquals =
+TokBuf.spelledForExpanded(llvm::ArrayRef(ExpandedEquals));
+if (!SpelledEquals)
+ return {};
+return {SpelledEquals->front().location(),
+FD->getDefaultLoc().getLocWithOffset(1)};
+ }
+}
+return {};
+ }
+
auto DeletionRange = FD->getBody()->getSourceRange();
if (auto *CD = llvm::dyn_cast(FD)) {
// AST doesn't contain the location for ":" in ctor initializers. Therefore
@@ -405,7 +438,9 @@ class DefineOutline : public Tweak {
return CodeAction::REFACTOR_KIND;
}
std::string title() const override {
-return "Move function body to out-of-line";
+if (Source->doesThisDeclarationHaveABody())
+ return "Move function body to out-of-line";
+return "Create function body out-of-line";
}
bool prepare(const Selection &Sel) override {
@@ -417,9 +452,8 @@ class DefineOutline : public Tweak {
return false;
Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
-// Bail out if the selection is not a in-line function definition.
-if (!Source || !Source->doesThisDeclarationHaveABody() ||
-Source->isOutOfLine())
+// Bail out if the selection is not a function declaration.
+if (!Source || Source->isDeleted() || Source->isOutOfLine())
return false;
// Bail out if this is a function template or specialization, as their
@@ -483,12 +517,17 @@ class DefineOutline : public Tweak {
if (!Effect)
return Effect.takeError();
-tooling::Replacements HeaderUpdates(tooling::Replacement(
-Sel.AST->getSourceManager(),
-CharSourceRange::getTokenRange(*toHalfOpenFileRange(
-SM, Sel.AST->getLangOpts(),
-getDeletionRange(Source, Sel.AST->getTokens(,
-";"));
+tooling::Replacements HeaderUpdates;
+auto DeletionRange = getDeletionRange(Source, Sel.AST->getTokens());
+if (DeletionRange.isValid()) {
+ if (auto Error = HeaderUpdates.add(tooling::Replacement(
+ Sel.AST->getSourceManager(),
+ CharSourceRange::getTokenRange(*toHalfOpenFileRange(
+ SM, Sel.AST->getLangOpts(), DeletionRange)),
+ ";"))) {
+return Error;
+ }
+}
if (Source->isInlineSpecified()) {
auto DelInline =
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index d1e60b070f20e95..dede5f7c3c25e8b 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -28,9 +28,6 @@ TE
