[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

This revision was automatically updated to reflect the committed changes. Closed by commit rGdc4cd43904df: [clangd] Add a textual fallback for go-to-definition (authored by sammccall, committed by nridge). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge added a comment. (Also just to clarify: while I said on Discord that I already implemented exclusion of string literals, I actually ended up deferring that part to a follow-up because it wasn't working as I expected.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge added a comment. I should mention that in my local usage, I've found the restriction on no more than 3 results (even if they're not in the current file) to be somewhat limiting. For example, a comment can easily reference the name of a function which has more than 3 overloads. But we

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:436 +if (ScoredResults.size() >= 3) { + // Assume we don't have results from the current file, otherwise the + // findNearbyIdentifier() mechanism

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge updated this revision to Diff 250031. nridge added a comment. Tweak a comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72874/new/ https://reviews.llvm.org/D72874 Files: clang-tools-extra/clangd/FindSymbols.cpp

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge marked 2 inline comments as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:436 +if (ScoredResults.size() >= 3) { + // Assume we don't have results from the current file, otherwise the + // findNearbyIdentifier() mechanism

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge updated this revision to Diff 250018. nridge marked 9 inline comments as done. nridge added a comment. Herald added a subscriber: mgrang. - Remove fuzzy matching - Rebase to apply to head, taking only the parts from D75479 that I need for index-based

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. In D72874#1915840 , @nridge wrote: > I've started to update the patch to be in line with the direction discussed > in the issue. > > @sammccall,

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge added a comment. I've started to update the patch to be in line with the direction discussed in the issue. @sammccall, how would you like to proceed logistically: - Do you plan to land (a possibly modified version of) D75479 ? - Or should I combine that

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:258 + +// For now, only consider exact name matches, including case. +// This is to avoid too many false positives. sammccall wrote: >

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall added a comment. I'd like to sync up briefly on https://github.com/clangd/clangd/issues/241 so we know where we want to end up. I think this is in good shape and certainly doesn't need a bigger scope, just want to be able to reason about how things will fit together.

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:489 + Index->fuzzyFind(Req, [&](const Symbol ) { +auto MaybeDeclLoc = +symbolLocationToLocation(Sym.CanonicalDeclaration, MainFilePath);

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge marked an inline comment as done. nridge added a comment. In D72874#1900648 , @sammccall wrote: > This reminds me: it's not completely obvious what set of "act on symbol under > the cursor" things this should (eventually) apply to. > I think not

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge updated this revision to Diff 248630. nridge marked 16 inline comments as done. nridge added a comment. Rebase onto D75479 and address most review comments Comments remaining to be addressed: - revising the tests to exercise

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge marked 6 inline comments as done. nridge added a comment. I'm posting some partial responses because I have some questions (really just one, about fuzzy matching). In general the comments seem reasonable and I plan to address all of them. (I've marked some comments as done because I've

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall added a comment. In D72874#1901606 , @nridge wrote: > The "dependent code" use case is a pretty important one in my eyes. > > In one of the codebases I work on, we have a fair amount of code like this: Yep, fair enough. And I don't think that

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge added a comment. Thanks for all the comments Sam! I'll have a detailed look tomorrow, but I wanted to follow up on this: In D72874#1901383 , @sammccall wrote: > I think having this trigger where the identifier is an actual token in the > program

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall added inline comments. Comment at: clang-tools-extra/clangd/SourceCode.cpp:313 SourceLocation getBeginningOfIdentifier(const Position , const SourceManager , sammccall wrote: > @kadircet is working on getting

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall added inline comments. Comment at: clang-tools-extra/clangd/SourceCode.cpp:313 SourceLocation getBeginningOfIdentifier(const Position , const SourceManager , sammccall wrote: > sammccall wrote: > > @kadircet

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall added a comment. I'm playing with a prototype of the token-based approach, a couple of follow-ups from that. I've split out functions to handle file/macro/ast from locateSymbolAt in e7de00cf974a4e30d4900518ae8473a117efbd6c

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall added a comment. Thanks! The scope looks good to me now, on to implementation details. I'm being a bit picky on the behaivor because go-to-def is a heavily-used feature, many users won't be expecting what we're doing here, and we can't reasonably expect them to understand the failure

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge added a comment. I've addressed some of the review comments, with a view to getting something minimal we can land, and improve on in follow-up changes. Mostly, I focused on the suggestions which reduce the number of results. I've left other suggestions which increase the number of

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge updated this revision to Diff 247529. nridge added a comment. Address some review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72874/new/ https://reviews.llvm.org/D72874 Files: clang-tools-extra/clangd/SourceCode.cpp

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge planned changes to this revision. nridge added a comment. Thanks for taking a look! In D72874#1831977 , @sammccall wrote: > - it triggers on almost every word, even words that plainly don't refer to > any decl like `format [[lazily]], in case

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

sammccall added a comment. I've tried this out locally and it's fun! As suspected on the bug though, IMO it's far from accurate enough. Examples from clangd/Compiler.cpp: - it triggers on almost every word, even words that plainly don't refer to any decl like `format [[lazily]], in case vlog

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge marked an inline comment as done. nridge added inline comments. Comment at: clang-tools-extra/clangd/XRefs.cpp:203 + if (!Symbols) { +elog("Workspace symbols failed", Symbols.takeError()); + } (There should be a return here, will fix locally.)

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 61850 tests passed, 0 failed and 781 were skipped. {icon question-circle color=gray} clang-tidy: unknown. {icon check-circle color=green} clang-format: pass. Build artifacts

[PATCH] D72874: [clangd] Add a textual fallback for go-to-definition

nridge created this revision. nridge added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. This facilitates performing go-to-definition inside comments, strings, invalid code, and