[clangd] Find better insertion locations in DefineOutline tweak #128164
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra Author: Christian Kandeler (ckandeler) ChangesIf possible, put the definition next to the definition of an adjacent declaration. For example: struct S { // S::foo1() goes here Patch is 22.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/128164.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 780aaa471dc8b..043d5c4f68c5e 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -1217,6 +1217,26 @@ EligibleRegion getEligiblePoints(llvm::StringRef Code,
return ER;
}
+std::string getNamespaceAtPosition(StringRef Code, const Position &Pos,
+ const LangOptions &LangOpts) {
+ std::vector<std::string> Enclosing = {""};
+ parseNamespaceEvents(Code, LangOpts, [&](NamespaceEvent Event) {
+ if (Pos < Event.Pos)
+ return;
+ if (Event.Trigger == NamespaceEvent::UsingDirective)
+ return;
+ if (!Event.Payload.empty())
+ Event.Payload.append("::");
+ if (Event.Trigger == NamespaceEvent::BeginNamespace) {
+ Enclosing.emplace_back(std::move(Event.Payload));
+ } else {
+ Enclosing.pop_back();
+ assert(Enclosing.back() == Event.Payload);
+ }
+ });
+ return Enclosing.back();
+}
+
bool isHeaderFile(llvm::StringRef FileName,
std::optional<LangOptions> LangOpts) {
// Respect the langOpts, for non-file-extension cases, e.g. standard library
diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h
index 028549f659d60..78ebc7a7d3b22 100644
--- a/clang-tools-extra/clangd/SourceCode.h
+++ b/clang-tools-extra/clangd/SourceCode.h
@@ -309,6 +309,9 @@ EligibleRegion getEligiblePoints(llvm::StringRef Code,
llvm::StringRef FullyQualifiedName,
const LangOptions &LangOpts);
+std::string getNamespaceAtPosition(llvm::StringRef Code, const Position &Pos,
+ const LangOptions &LangOpts);
+
struct DefinedMacro {
llvm::StringRef Name;
const MacroInfo *Info;
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index e4eef228b6b99..5ff64aa946004 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "AST.h"
+#include "FindSymbols.h"
#include "FindTarget.h"
#include "HeaderSourceSwitch.h"
#include "ParsedAST.h"
@@ -489,8 +490,16 @@ class DefineOutline : public Tweak {
Expected<Effect> apply(const Selection &Sel) override {
const SourceManager &SM = Sel.AST->getSourceManager();
- auto CCFile = SameFile ? Sel.AST->tuPath().str()
- : getSourceFile(Sel.AST->tuPath(), Sel);
+ std::optional<Path> CCFile;
+ std::optional<std::pair<Symbol, RelativeInsertPos>> Anchor =
+ getDefinitionOfAdjacentDecl(Sel);
+ if (Anchor) {
+ CCFile = Anchor->first.Definition.FileURI;
+ CCFile->erase(0, 7); // "file://"
+ } else {
+ 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");
@@ -499,21 +508,62 @@ class DefineOutline : public Tweak {
// doesn't exist?
if (!Buffer)
return llvm::errorCodeToError(Buffer.getError());
+
+ std::optional<Position> InsertionPos;
+ if (Anchor) {
+ if (auto P = getInsertionPointFromExistingDefinition(
+ **Buffer, Anchor->first, Anchor->second, Sel.AST)) {
+ InsertionPos = *P;
+ }
+ }
+
auto Contents = Buffer->get()->getBuffer();
- auto InsertionPoint = getInsertionPoint(Contents, Sel);
- if (!InsertionPoint)
- return InsertionPoint.takeError();
+
+ std::size_t Offset = -1;
+ const DeclContext *EnclosingNamespace = nullptr;
+ std::string EnclosingNamespaceName;
+
+ if (InsertionPos) {
+ EnclosingNamespaceName = getNamespaceAtPosition(Contents, *InsertionPos,
+ Sel.AST->getLangOpts());
+ } else if (SameFile) {
+ auto P = getInsertionPointInMainFile(Sel.AST);
+ if (!P)
+ return P.takeError();
+ Offset = P->Offset;
+ EnclosingNamespace = P->EnclosingNamespace;
+ } else {
+ auto Region = getEligiblePoints(
+ Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+ assert(!Region.EligiblePoints.empty());
+ EnclosingNamespaceName = Region.EnclosingNamespace;
+ InsertionPos = Region.EligiblePoints.back();
+ }
+
+ if (InsertionPos) {
+ auto O = positionToOffset(Contents, *InsertionPos);
+ if (!O)
+ return O.takeError();
+ Offset = *O;
+ auto TargetContext =
+ findContextForNS(EnclosingNamespaceName, Source->getDeclContext());
+ if (!TargetContext)
+ return error("define outline: couldn't find a context for target");
+ EnclosingNamespace = *TargetContext;
+ }
+
+ assert(Offset >= 0);
+ assert(EnclosingNamespace);
auto FuncDef = getFunctionSourceCode(
- Source, InsertionPoint->EnclosingNamespace, Sel.AST->getTokens(),
+ Source, EnclosingNamespace, Sel.AST->getTokens(),
Sel.AST->getHeuristicResolver(),
SameFile && isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts()));
if (!FuncDef)
return FuncDef.takeError();
SourceManagerForFile SMFF(*CCFile, Contents);
- const tooling::Replacement InsertFunctionDef(
- *CCFile, InsertionPoint->Offset, 0, *FuncDef);
+ const tooling::Replacement InsertFunctionDef(*CCFile, Offset, 0, *FuncDef);
auto Effect = Effect::mainFileEdit(
SMFF.get(), tooling::Replacements(InsertFunctionDef));
if (!Effect)
@@ -548,59 +598,191 @@ class DefineOutline : public Tweak {
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;
+ enum class RelativeInsertPos { Before, After };
+ std::optional<std::pair<Symbol, RelativeInsertPos>>
+ getDefinitionOfAdjacentDecl(const Selection &Sel) {
+ if (!Sel.Index)
+ return {};
+ std::optional<Symbol> Anchor;
+ Path PrefixedTuPath = "file://" + Sel.AST->tuPath().str();
+ auto CheckCandidate = [&](Decl *Candidate) {
+ assert(Candidate != Source);
+ if (auto Func = llvm::dyn_cast_or_null<FunctionDecl>(Candidate);
+ !Func || Func->isThisDeclarationADefinition()) {
+ return;
+ }
+ std::optional<Symbol> CandidateSymbol;
+ Sel.Index->lookup({{getSymbolID(Candidate)}}, [&](const Symbol &S) {
+ if (S.Definition) {
+ CandidateSymbol = S;
}
- unsigned Offset = Sel.AST->getSourceManager()
- .getDecomposedLoc(It->endLocation())
- .second;
- return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset};
+ });
+ if (!CandidateSymbol)
+ return;
+
+ // If our definition is constrained to the same file, ignore
+ // definitions that are not located there.
+ // If our definition is not constrained to the same file, but
+ // our anchor definition is in the same file, then we also put our
+ // definition there, because that appears to be the user preference.
+ // Exception: If the existing definition is a template, then the
+ // location is likely due to technical necessity rather than preference,
+ // so ignore that definition.
+ bool CandidateSameFile =
+ PrefixedTuPath == CandidateSymbol->Definition.FileURI;
+ if (SameFile && !CandidateSameFile)
+ return;
+ if (!SameFile && CandidateSameFile) {
+ if (Candidate->isTemplateDecl())
+ return;
+ SameFile = true;
}
- return error(
- "failed to determine insertion location: no end of class found");
+ Anchor = *CandidateSymbol;
+ };
+
+ // Try to find adjacent function declarations.
+ // Determine the closest one by alternatingly going "up" and "down"
+ // from our function in increasing steps.
+ const DeclContext *ParentContext = Source->getParent();
+ const auto SourceIt = llvm::find_if(
+ ParentContext->decls(), [this](const Decl *D) { return D == Source; });
+ if (SourceIt == ParentContext->decls_end())
+ return {};
+ const int Preceding = std::distance(ParentContext->decls_begin(), SourceIt);
+ const int Following =
+ std::distance(SourceIt, ParentContext->decls_end()) - 1;
+ for (int Offset = 1; Offset <= Preceding || Offset <= Following; ++Offset) {
+ if (Offset <= Preceding)
+ CheckCandidate(
+ *std::next(ParentContext->decls_begin(), Preceding - Offset));
+ if (Anchor)
+ return std::make_pair(*Anchor, RelativeInsertPos::After);
+ if (Offset <= Following)
+ CheckCandidate(*std::next(SourceIt, Offset));
+ if (Anchor)
+ return std::make_pair(*Anchor, RelativeInsertPos::Before);
}
+ return {};
+ }
- auto Region = getEligiblePoints(
- Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+ // We don't know the actual start or end of the definition, only the position
+ // of the name. Therefore, we heuristically try to locate the last token
+ // before or in this function, respectively. Adapt as required by user code.
+ llvm::Expected<Position> getInsertionPointFromExistingDefinition(
+ const llvm::MemoryBuffer &Buffer, const Symbol &S,
+ RelativeInsertPos RelInsertPos, ParsedAST *AST) {
+ auto LspPos = indexToLSPLocation(S.Definition, AST->tuPath());
+ if (!LspPos)
+ return LspPos.takeError();
+ auto StartOffset =
+ positionToOffset(Buffer.getBuffer(), LspPos->range.start);
+ if (!StartOffset)
+ return LspPos.takeError();
+ SourceLocation InsertionLoc;
+ SourceManager &SM = AST->getSourceManager();
+ FileID F = Buffer.getBufferIdentifier() == AST->tuPath()
+ ? SM.getMainFileID()
+ : SM.createFileID(Buffer);
+
+ auto InsertBefore = [&] {
+ // Go backwards until we encounter one of the following:
+ // - An opening brace (of a namespace).
+ // - A closing brace (of a function definition).
+ // - A semicolon (of a declaration).
+ // If no such token was found, then the first token in the file starts the
+ // definition.
+ auto Tokens = syntax::tokenize(syntax::FileRange(F, 0, *StartOffset), SM,
+ AST->getLangOpts());
+ if (Tokens.empty())
+ return;
+ for (auto I = std::rbegin(Tokens);
+ InsertionLoc.isInvalid() && I != std::rend(Tokens); ++I) {
+ switch (I->kind()) {
+ case tok::l_brace:
+ case tok::r_brace:
+ case tok::semi:
+ if (I != std::rbegin(Tokens))
+ InsertionLoc = std::prev(I)->location();
+ else
+ InsertionLoc = I->endLocation();
+ break;
+ default:
+ break;
+ }
+ }
+ if (InsertionLoc.isInvalid())
+ InsertionLoc = Tokens.front().location();
+ };
- assert(!Region.EligiblePoints.empty());
- auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
- if (!Offset)
- return Offset.takeError();
+ if (RelInsertPos == RelativeInsertPos::Before) {
+ InsertBefore();
+ } else {
+ // Skip over one top-level pair of parentheses (for the parameter list)
+ // and one pair of curly braces (for the code block).
+ // If that fails, insert before the function instead.
+ auto Tokens = syntax::tokenize(
+ syntax::FileRange(F, *StartOffset, Buffer.getBuffer().size()), SM,
+ AST->getLangOpts());
+ bool SkippedParams = false;
+ int OpenParens = 0;
+ int OpenBraces = 0;
+ std::optional<syntax::Token> Tok;
+ for (const auto &T : Tokens) {
+ tok::TokenKind StartKind = SkippedParams ? tok::l_brace : tok::l_paren;
+ tok::TokenKind EndKind = SkippedParams ? tok::r_brace : tok::r_paren;
+ int &Count = SkippedParams ? OpenBraces : OpenParens;
+ if (T.kind() == StartKind) {
+ ++Count;
+ } else if (T.kind() == EndKind) {
+ if (--Count == 0) {
+ if (SkippedParams) {
+ Tok = T;
+ break;
+ }
+ SkippedParams = true;
+ } else if (Count < 0) {
+ break;
+ }
+ }
+ }
+ if (Tok)
+ InsertionLoc = Tok->endLocation();
+ else
+ InsertBefore();
+ }
- auto TargetContext =
- findContextForNS(Region.EnclosingNamespace, Source->getDeclContext());
- if (!TargetContext)
- return error("define outline: couldn't find a context for target");
+ return InsertionLoc.isValid() ? sourceLocToPosition(SM, InsertionLoc)
+ : Position();
+ }
- return InsertionPoint{*TargetContext, *Offset};
+ // Returns the most natural insertion point in this file.
+ // This is a fallback for when we failed to find an existing definition to
+ // place the new one next to. It only considers namespace proximity.
+ llvm::Expected<InsertionPoint> getInsertionPointInMainFile(ParsedAST *AST) {
+ // 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.
+ 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 = 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 =
+ AST->getSourceManager().getDecomposedLoc(It->endLocation()).second;
+ return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset};
+ }
+ return error(
+ "failed to determine insertion location: no end of class found");
}
private:
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index b5f09f9b14604..9340255543421 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -7,7 +7,9 @@
//===----------------------------------------------------------------------===//
#include "TestFS.h"
+#include "TestIndex.h"
#include "TweakTesting.h"
+#include "index/MemIndex.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -447,9 +449,27 @@ inline T Foo<T>::bar(const T& t, const U& u) { return {}; }
TEST_F(DefineOutlineTest, InCppFile) {
FileName = "Test.cpp";
+ const auto MakePos = [](uint32_t Line, uint32_t Col) {
+ SymbolLocation::Position Pos;
+ Pos.setLine(Line);
+ Pos.setColumn(Col);
+ return Pos;
+ };
+ struct SymbolSpec {
+ StringRef NamespaceName;
+ StringRef ClassName;
+ StringRef FuncName;
+ SymbolLocation::Position DeclStart;
+ SymbolLocation::Position DeclEnd;
+ SymbolLocation::Position DefStart;
+ SymbolLocation::Position DefEnd;
+ };
+ using SymbolSpecs = std::vector<SymbolSpec>;
+
struct {
llvm::StringRef Test;
llvm::StringRef ExpectedSource;
+ SymbolSpecs Definitions;
} Cases[] = {
{
R"cpp(
@@ -470,10 +490,163 @@ TEST_F(DefineOutlineTest, InCppFile) {
}
}
)cpp"},
+
+ // Criterion 1: Distance
+ {
+ R"cpp(
+ struct Foo {
+ void ignored1(); // Too far away
+ void ignored2(); // No definition
+ void ignored3() {} // Defined inline
+ void fo^o() {}
+ void neighbor();
+ };
+ void Foo::ignored1() {}
+ void Foo::neighbor() {}
+ )cpp",
+ R"cpp(
+ struct Foo {
+ void ignored1(); // Too far away
+ void ignored2(); // No definition
+ void ignored3() {} // Defined inline
+ void foo() ;
+ void neighbor();
+ };
+ void Foo::ignored1() {}
+ void Foo::foo() {}
+void Foo::neighbor() {}
+ )cpp",
+ SymbolSpecs{{"", "Foo", "ignored3", MakePos(4, 21), MakePos(4, 29),
+ MakePos(4, 21), MakePos(4, 29)},
+ {"", "Foo", "ignored1", MakePos(2, 21), MakePos(2, 29),
+ MakePos(8, 22), MakePos(8, 30)},
+ {"", "Foo", "neighbor", MakePos(6, 21), MakePos(6, 29),
+ MakePos(9, 22), MakePos(9, 30)}}},
+
+ // Criterion 2: Prefer preceding
+ {
+ R"cpp(
+ struct Foo {
+ void neighbor();
+ void fo^o() {}
+ void ignored();
+ };
+ void Foo::neighbor() {}
+ void Foo::ignored() {}
+ )cpp",
+ R"cpp(
+ struct Foo {
+ void neighbor();
+ void foo() ;
+ void ignored();
+ };
+ void Foo::neighbor() {}void Foo::foo() {}
+
+ void Foo::ignored() {}
+ )cpp",
+ SymbolSpecs{{"", "Foo", "ignored", MakePos(4, 21), MakePos(4, 28),
+ MakePos(7, 22), MakePos(7, 29)},
+ {"", "Foo", "neighbor", MakePos(2, 22), MakePos(2, 29),
+ MakePos(6, 22), MakePos(6, 30)}}},
+
+ // Like above, but with a namespace
+ {
+ R"cpp(
+ namespace NS {
+ struct Foo {
+ void neighbor();
+ void fo^o() {}
+ void ignored();
+ };
+ void Foo::neighbor() {}
+ void Foo::ignored() {}
+ }
+ )cpp",
+ R"cpp(
+ namespace NS {
+ struct Foo {
+ void neighbor();
+ void foo() ;
+ void ignored();
+ };
+ void Foo::neighbor() {}void Foo::foo() {}
+
+ void Foo::ignored() {}
+ }
+ )cpp",
+ SymbolSpecs{{"NS", "Foo", "ignored", MakePos(5, 21), MakePos(5, 29),
+ MakePos(8, 22), MakePos(8, 30)},
+ {"NS", "Foo", "neighbor", MakePos(3, 21), MakePos(3...
[truncated]
|
|
@llvm/pr-subscribers-clangd Author: Christian Kandeler (ckandeler) ChangesIf possible, put the definition next to the definition of an adjacent declaration. For example: struct S { // S::foo1() goes here Patch is 22.35 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/128164.diff 4 Files Affected:
diff --git a/clang-tools-extra/clangd/SourceCode.cpp b/clang-tools-extra/clangd/SourceCode.cpp
index 780aaa471dc8b..043d5c4f68c5e 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -1217,6 +1217,26 @@ EligibleRegion getEligiblePoints(llvm::StringRef Code,
return ER;
}
+std::string getNamespaceAtPosition(StringRef Code, const Position &Pos,
+ const LangOptions &LangOpts) {
+ std::vector<std::string> Enclosing = {""};
+ parseNamespaceEvents(Code, LangOpts, [&](NamespaceEvent Event) {
+ if (Pos < Event.Pos)
+ return;
+ if (Event.Trigger == NamespaceEvent::UsingDirective)
+ return;
+ if (!Event.Payload.empty())
+ Event.Payload.append("::");
+ if (Event.Trigger == NamespaceEvent::BeginNamespace) {
+ Enclosing.emplace_back(std::move(Event.Payload));
+ } else {
+ Enclosing.pop_back();
+ assert(Enclosing.back() == Event.Payload);
+ }
+ });
+ return Enclosing.back();
+}
+
bool isHeaderFile(llvm::StringRef FileName,
std::optional<LangOptions> LangOpts) {
// Respect the langOpts, for non-file-extension cases, e.g. standard library
diff --git a/clang-tools-extra/clangd/SourceCode.h b/clang-tools-extra/clangd/SourceCode.h
index 028549f659d60..78ebc7a7d3b22 100644
--- a/clang-tools-extra/clangd/SourceCode.h
+++ b/clang-tools-extra/clangd/SourceCode.h
@@ -309,6 +309,9 @@ EligibleRegion getEligiblePoints(llvm::StringRef Code,
llvm::StringRef FullyQualifiedName,
const LangOptions &LangOpts);
+std::string getNamespaceAtPosition(llvm::StringRef Code, const Position &Pos,
+ const LangOptions &LangOpts);
+
struct DefinedMacro {
llvm::StringRef Name;
const MacroInfo *Info;
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
index e4eef228b6b99..5ff64aa946004 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "AST.h"
+#include "FindSymbols.h"
#include "FindTarget.h"
#include "HeaderSourceSwitch.h"
#include "ParsedAST.h"
@@ -489,8 +490,16 @@ class DefineOutline : public Tweak {
Expected<Effect> apply(const Selection &Sel) override {
const SourceManager &SM = Sel.AST->getSourceManager();
- auto CCFile = SameFile ? Sel.AST->tuPath().str()
- : getSourceFile(Sel.AST->tuPath(), Sel);
+ std::optional<Path> CCFile;
+ std::optional<std::pair<Symbol, RelativeInsertPos>> Anchor =
+ getDefinitionOfAdjacentDecl(Sel);
+ if (Anchor) {
+ CCFile = Anchor->first.Definition.FileURI;
+ CCFile->erase(0, 7); // "file://"
+ } else {
+ 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");
@@ -499,21 +508,62 @@ class DefineOutline : public Tweak {
// doesn't exist?
if (!Buffer)
return llvm::errorCodeToError(Buffer.getError());
+
+ std::optional<Position> InsertionPos;
+ if (Anchor) {
+ if (auto P = getInsertionPointFromExistingDefinition(
+ **Buffer, Anchor->first, Anchor->second, Sel.AST)) {
+ InsertionPos = *P;
+ }
+ }
+
auto Contents = Buffer->get()->getBuffer();
- auto InsertionPoint = getInsertionPoint(Contents, Sel);
- if (!InsertionPoint)
- return InsertionPoint.takeError();
+
+ std::size_t Offset = -1;
+ const DeclContext *EnclosingNamespace = nullptr;
+ std::string EnclosingNamespaceName;
+
+ if (InsertionPos) {
+ EnclosingNamespaceName = getNamespaceAtPosition(Contents, *InsertionPos,
+ Sel.AST->getLangOpts());
+ } else if (SameFile) {
+ auto P = getInsertionPointInMainFile(Sel.AST);
+ if (!P)
+ return P.takeError();
+ Offset = P->Offset;
+ EnclosingNamespace = P->EnclosingNamespace;
+ } else {
+ auto Region = getEligiblePoints(
+ Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+ assert(!Region.EligiblePoints.empty());
+ EnclosingNamespaceName = Region.EnclosingNamespace;
+ InsertionPos = Region.EligiblePoints.back();
+ }
+
+ if (InsertionPos) {
+ auto O = positionToOffset(Contents, *InsertionPos);
+ if (!O)
+ return O.takeError();
+ Offset = *O;
+ auto TargetContext =
+ findContextForNS(EnclosingNamespaceName, Source->getDeclContext());
+ if (!TargetContext)
+ return error("define outline: couldn't find a context for target");
+ EnclosingNamespace = *TargetContext;
+ }
+
+ assert(Offset >= 0);
+ assert(EnclosingNamespace);
auto FuncDef = getFunctionSourceCode(
- Source, InsertionPoint->EnclosingNamespace, Sel.AST->getTokens(),
+ Source, EnclosingNamespace, Sel.AST->getTokens(),
Sel.AST->getHeuristicResolver(),
SameFile && isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts()));
if (!FuncDef)
return FuncDef.takeError();
SourceManagerForFile SMFF(*CCFile, Contents);
- const tooling::Replacement InsertFunctionDef(
- *CCFile, InsertionPoint->Offset, 0, *FuncDef);
+ const tooling::Replacement InsertFunctionDef(*CCFile, Offset, 0, *FuncDef);
auto Effect = Effect::mainFileEdit(
SMFF.get(), tooling::Replacements(InsertFunctionDef));
if (!Effect)
@@ -548,59 +598,191 @@ class DefineOutline : public Tweak {
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;
+ enum class RelativeInsertPos { Before, After };
+ std::optional<std::pair<Symbol, RelativeInsertPos>>
+ getDefinitionOfAdjacentDecl(const Selection &Sel) {
+ if (!Sel.Index)
+ return {};
+ std::optional<Symbol> Anchor;
+ Path PrefixedTuPath = "file://" + Sel.AST->tuPath().str();
+ auto CheckCandidate = [&](Decl *Candidate) {
+ assert(Candidate != Source);
+ if (auto Func = llvm::dyn_cast_or_null<FunctionDecl>(Candidate);
+ !Func || Func->isThisDeclarationADefinition()) {
+ return;
+ }
+ std::optional<Symbol> CandidateSymbol;
+ Sel.Index->lookup({{getSymbolID(Candidate)}}, [&](const Symbol &S) {
+ if (S.Definition) {
+ CandidateSymbol = S;
}
- unsigned Offset = Sel.AST->getSourceManager()
- .getDecomposedLoc(It->endLocation())
- .second;
- return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset};
+ });
+ if (!CandidateSymbol)
+ return;
+
+ // If our definition is constrained to the same file, ignore
+ // definitions that are not located there.
+ // If our definition is not constrained to the same file, but
+ // our anchor definition is in the same file, then we also put our
+ // definition there, because that appears to be the user preference.
+ // Exception: If the existing definition is a template, then the
+ // location is likely due to technical necessity rather than preference,
+ // so ignore that definition.
+ bool CandidateSameFile =
+ PrefixedTuPath == CandidateSymbol->Definition.FileURI;
+ if (SameFile && !CandidateSameFile)
+ return;
+ if (!SameFile && CandidateSameFile) {
+ if (Candidate->isTemplateDecl())
+ return;
+ SameFile = true;
}
- return error(
- "failed to determine insertion location: no end of class found");
+ Anchor = *CandidateSymbol;
+ };
+
+ // Try to find adjacent function declarations.
+ // Determine the closest one by alternatingly going "up" and "down"
+ // from our function in increasing steps.
+ const DeclContext *ParentContext = Source->getParent();
+ const auto SourceIt = llvm::find_if(
+ ParentContext->decls(), [this](const Decl *D) { return D == Source; });
+ if (SourceIt == ParentContext->decls_end())
+ return {};
+ const int Preceding = std::distance(ParentContext->decls_begin(), SourceIt);
+ const int Following =
+ std::distance(SourceIt, ParentContext->decls_end()) - 1;
+ for (int Offset = 1; Offset <= Preceding || Offset <= Following; ++Offset) {
+ if (Offset <= Preceding)
+ CheckCandidate(
+ *std::next(ParentContext->decls_begin(), Preceding - Offset));
+ if (Anchor)
+ return std::make_pair(*Anchor, RelativeInsertPos::After);
+ if (Offset <= Following)
+ CheckCandidate(*std::next(SourceIt, Offset));
+ if (Anchor)
+ return std::make_pair(*Anchor, RelativeInsertPos::Before);
}
+ return {};
+ }
- auto Region = getEligiblePoints(
- Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+ // We don't know the actual start or end of the definition, only the position
+ // of the name. Therefore, we heuristically try to locate the last token
+ // before or in this function, respectively. Adapt as required by user code.
+ llvm::Expected<Position> getInsertionPointFromExistingDefinition(
+ const llvm::MemoryBuffer &Buffer, const Symbol &S,
+ RelativeInsertPos RelInsertPos, ParsedAST *AST) {
+ auto LspPos = indexToLSPLocation(S.Definition, AST->tuPath());
+ if (!LspPos)
+ return LspPos.takeError();
+ auto StartOffset =
+ positionToOffset(Buffer.getBuffer(), LspPos->range.start);
+ if (!StartOffset)
+ return LspPos.takeError();
+ SourceLocation InsertionLoc;
+ SourceManager &SM = AST->getSourceManager();
+ FileID F = Buffer.getBufferIdentifier() == AST->tuPath()
+ ? SM.getMainFileID()
+ : SM.createFileID(Buffer);
+
+ auto InsertBefore = [&] {
+ // Go backwards until we encounter one of the following:
+ // - An opening brace (of a namespace).
+ // - A closing brace (of a function definition).
+ // - A semicolon (of a declaration).
+ // If no such token was found, then the first token in the file starts the
+ // definition.
+ auto Tokens = syntax::tokenize(syntax::FileRange(F, 0, *StartOffset), SM,
+ AST->getLangOpts());
+ if (Tokens.empty())
+ return;
+ for (auto I = std::rbegin(Tokens);
+ InsertionLoc.isInvalid() && I != std::rend(Tokens); ++I) {
+ switch (I->kind()) {
+ case tok::l_brace:
+ case tok::r_brace:
+ case tok::semi:
+ if (I != std::rbegin(Tokens))
+ InsertionLoc = std::prev(I)->location();
+ else
+ InsertionLoc = I->endLocation();
+ break;
+ default:
+ break;
+ }
+ }
+ if (InsertionLoc.isInvalid())
+ InsertionLoc = Tokens.front().location();
+ };
- assert(!Region.EligiblePoints.empty());
- auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
- if (!Offset)
- return Offset.takeError();
+ if (RelInsertPos == RelativeInsertPos::Before) {
+ InsertBefore();
+ } else {
+ // Skip over one top-level pair of parentheses (for the parameter list)
+ // and one pair of curly braces (for the code block).
+ // If that fails, insert before the function instead.
+ auto Tokens = syntax::tokenize(
+ syntax::FileRange(F, *StartOffset, Buffer.getBuffer().size()), SM,
+ AST->getLangOpts());
+ bool SkippedParams = false;
+ int OpenParens = 0;
+ int OpenBraces = 0;
+ std::optional<syntax::Token> Tok;
+ for (const auto &T : Tokens) {
+ tok::TokenKind StartKind = SkippedParams ? tok::l_brace : tok::l_paren;
+ tok::TokenKind EndKind = SkippedParams ? tok::r_brace : tok::r_paren;
+ int &Count = SkippedParams ? OpenBraces : OpenParens;
+ if (T.kind() == StartKind) {
+ ++Count;
+ } else if (T.kind() == EndKind) {
+ if (--Count == 0) {
+ if (SkippedParams) {
+ Tok = T;
+ break;
+ }
+ SkippedParams = true;
+ } else if (Count < 0) {
+ break;
+ }
+ }
+ }
+ if (Tok)
+ InsertionLoc = Tok->endLocation();
+ else
+ InsertBefore();
+ }
- auto TargetContext =
- findContextForNS(Region.EnclosingNamespace, Source->getDeclContext());
- if (!TargetContext)
- return error("define outline: couldn't find a context for target");
+ return InsertionLoc.isValid() ? sourceLocToPosition(SM, InsertionLoc)
+ : Position();
+ }
- return InsertionPoint{*TargetContext, *Offset};
+ // Returns the most natural insertion point in this file.
+ // This is a fallback for when we failed to find an existing definition to
+ // place the new one next to. It only considers namespace proximity.
+ llvm::Expected<InsertionPoint> getInsertionPointInMainFile(ParsedAST *AST) {
+ // 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.
+ 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 = 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 =
+ AST->getSourceManager().getDecomposedLoc(It->endLocation()).second;
+ return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset};
+ }
+ return error(
+ "failed to determine insertion location: no end of class found");
}
private:
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
index b5f09f9b14604..9340255543421 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -7,7 +7,9 @@
//===----------------------------------------------------------------------===//
#include "TestFS.h"
+#include "TestIndex.h"
#include "TweakTesting.h"
+#include "index/MemIndex.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
@@ -447,9 +449,27 @@ inline T Foo<T>::bar(const T& t, const U& u) { return {}; }
TEST_F(DefineOutlineTest, InCppFile) {
FileName = "Test.cpp";
+ const auto MakePos = [](uint32_t Line, uint32_t Col) {
+ SymbolLocation::Position Pos;
+ Pos.setLine(Line);
+ Pos.setColumn(Col);
+ return Pos;
+ };
+ struct SymbolSpec {
+ StringRef NamespaceName;
+ StringRef ClassName;
+ StringRef FuncName;
+ SymbolLocation::Position DeclStart;
+ SymbolLocation::Position DeclEnd;
+ SymbolLocation::Position DefStart;
+ SymbolLocation::Position DefEnd;
+ };
+ using SymbolSpecs = std::vector<SymbolSpec>;
+
struct {
llvm::StringRef Test;
llvm::StringRef ExpectedSource;
+ SymbolSpecs Definitions;
} Cases[] = {
{
R"cpp(
@@ -470,10 +490,163 @@ TEST_F(DefineOutlineTest, InCppFile) {
}
}
)cpp"},
+
+ // Criterion 1: Distance
+ {
+ R"cpp(
+ struct Foo {
+ void ignored1(); // Too far away
+ void ignored2(); // No definition
+ void ignored3() {} // Defined inline
+ void fo^o() {}
+ void neighbor();
+ };
+ void Foo::ignored1() {}
+ void Foo::neighbor() {}
+ )cpp",
+ R"cpp(
+ struct Foo {
+ void ignored1(); // Too far away
+ void ignored2(); // No definition
+ void ignored3() {} // Defined inline
+ void foo() ;
+ void neighbor();
+ };
+ void Foo::ignored1() {}
+ void Foo::foo() {}
+void Foo::neighbor() {}
+ )cpp",
+ SymbolSpecs{{"", "Foo", "ignored3", MakePos(4, 21), MakePos(4, 29),
+ MakePos(4, 21), MakePos(4, 29)},
+ {"", "Foo", "ignored1", MakePos(2, 21), MakePos(2, 29),
+ MakePos(8, 22), MakePos(8, 30)},
+ {"", "Foo", "neighbor", MakePos(6, 21), MakePos(6, 29),
+ MakePos(9, 22), MakePos(9, 30)}}},
+
+ // Criterion 2: Prefer preceding
+ {
+ R"cpp(
+ struct Foo {
+ void neighbor();
+ void fo^o() {}
+ void ignored();
+ };
+ void Foo::neighbor() {}
+ void Foo::ignored() {}
+ )cpp",
+ R"cpp(
+ struct Foo {
+ void neighbor();
+ void foo() ;
+ void ignored();
+ };
+ void Foo::neighbor() {}void Foo::foo() {}
+
+ void Foo::ignored() {}
+ )cpp",
+ SymbolSpecs{{"", "Foo", "ignored", MakePos(4, 21), MakePos(4, 28),
+ MakePos(7, 22), MakePos(7, 29)},
+ {"", "Foo", "neighbor", MakePos(2, 22), MakePos(2, 29),
+ MakePos(6, 22), MakePos(6, 30)}}},
+
+ // Like above, but with a namespace
+ {
+ R"cpp(
+ namespace NS {
+ struct Foo {
+ void neighbor();
+ void fo^o() {}
+ void ignored();
+ };
+ void Foo::neighbor() {}
+ void Foo::ignored() {}
+ }
+ )cpp",
+ R"cpp(
+ namespace NS {
+ struct Foo {
+ void neighbor();
+ void foo() ;
+ void ignored();
+ };
+ void Foo::neighbor() {}void Foo::foo() {}
+
+ void Foo::ignored() {}
+ }
+ )cpp",
+ SymbolSpecs{{"NS", "Foo", "ignored", MakePos(5, 21), MakePos(5, 29),
+ MakePos(8, 22), MakePos(8, 30)},
+ {"NS", "Foo", "neighbor", MakePos(3, 21), MakePos(3...
[truncated]
|
ckandeler
force-pushed
the
better-insert-locations
branch
5 times, most recently
from
da6a6e8 to
cca5b5a
Compare
If possible, put the definition next to the definition of an adjacent
declaration. For example:
struct S {
void f^oo1() {}
void foo2();
void f^oo3() {}
};
// S::foo1() goes here
void S::foo2() {}
// S::foo3() goes here
ckandeler
force-pushed
the
better-insert-locations
branch
from
cca5b5a to
87069ad
Compare
|
ping |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If possible, put the definition next to the definition of an adjacent declaration. For example:
struct S {
void f^oo1() {}
void foo2();
void f^oo3() {}
};
// S::foo1() goes here
void S::foo2() {}
// S::foo3() goes here