https://github.com/ckandeler updated https://github.com/llvm/llvm-project/pull/220518
>From 7d6b09b9438f26434bc73fb266d95afa47ccb5db Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Tue, 1 Sep 2026 18:44:14 +0200 Subject: [PATCH 1/4] [clangd] Include the operator name in documentHighlight Placing the cursor on an overloaded operator's declaration (e.g. `operator new`, `operator[]`) and requesting textDocument/document Highlight only highlighted the `operator` keyword itself, not the name or symbol that follows it (`new`, `[]`, etc.), even though that name is what's actually significant to the user. ReferenceFinder already splits some references into several spelled tokens (used for Objective-C's split selector syntax); reuse that mechanism for the operator name too, extracting the tokens spelled in the operator name's source range. This only applies to the declaration's own occurrence. Assisted-by: Claude --- clang-tools-extra/clangd/XRefs.cpp | 37 +++++++++++++++++++ .../clangd/unittests/XRefsTests.cpp | 26 +++++++++++++ 2 files changed, 63 insertions(+) diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index 73d8eb5d00569..bbaaf1305823c 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -1014,6 +1014,30 @@ std::vector<DocumentLink> getDocumentLinks(ParsedAST &AST) { namespace { +/// Returns the locations of the spelled tokens overlapping [Range.getBegin(), +/// Range.getEnd()], in order. Both ends of \p Range must be file locations +/// in the same file. +llvm::SmallVector<SourceLocation, 4> +tokensSpelledInRange(const syntax::TokenBuffer &TB, const SourceManager &SM, + SourceRange Range) { + llvm::SmallVector<SourceLocation, 4> Locs; + if (Range.getBegin().isInvalid() || Range.getEnd().isInvalid()) + return Locs; + FileID FID = SM.getFileID(Range.getBegin()); + if (FID != SM.getFileID(Range.getEnd())) + return Locs; + unsigned EndOffset = SM.getFileOffset(Range.getEnd()); + llvm::ArrayRef<syntax::Token> Toks = TB.spelledTokens(FID); + auto It = llvm::partition_point(Toks, [&](const syntax::Token &Tok) { + return SM.getFileOffset(Tok.location()) < + SM.getFileOffset(Range.getBegin()); + }); + for (; It != Toks.end() && SM.getFileOffset(It->location()) <= EndOffset; + ++It) + Locs.push_back(It->location()); + return Locs; +} + /// Collects references to symbols within the main file. class ReferenceFinder : public index::IndexDataConsumer { public: @@ -1091,6 +1115,19 @@ class ReferenceFinder : public index::IndexDataConsumer { } else if (auto *OMD = llvm::dyn_cast_or_null<ObjCMethodDecl>(ASTNode.OrigD)) { OMD->getSelectorLocs(Locs); + } else if (auto *FD = llvm::dyn_cast_or_null<FunctionDecl>(D); + FD && FD->isOverloadedOperator() && + isInsideMainFile(FD->getNameInfo().getLoc(), SM)) { + // The operator name (e.g. `new`, `[]`, `<<`) is a separate token (or + // tokens) from the `operator` keyword itself; report both so the + // whole name gets highlighted, not just the keyword. Only do this + // when the declaration itself is in the main file: TB only has + // spelled tokens for the main file, and this is only useful anyway + // when we're looking at the occurrence at the declaration itself + // (checked below). + Locs.push_back(FD->getNameInfo().getLoc()); + auto OpNameRange = FD->getNameInfo().getCXXOperatorNameRange(); + llvm::append_range(Locs, tokensSpelledInRange(TB, SM, OpNameRange)); } // Sanity check: we expect the *first* token to match the reported loc. // Otherwise, maybe it was e.g. some other kind of reference to a Decl. diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index d5ba2bc093c9c..5641b716f552f 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -135,6 +135,32 @@ TEST(HighlightsTest, All) { return 1; } )cpp", + R"cpp(// Overloaded operator: the whole name, not just `operator`, is highlighted. + using size_t = decltype(sizeof(0)); + struct S { + static void *[[operator]] [[n^ew]](size_t); + static void operator delete(void *); + }; + )cpp", + R"cpp(// Same, with the cursor on the operator keyword itself. + using size_t = decltype(sizeof(0)); + struct S { + static void *[[^operator]] [[new]](size_t); + static void operator delete(void *); + }; + )cpp", + R"cpp(// Same, for operator delete. + using size_t = decltype(sizeof(0)); + struct S { + static void *operator new(size_t); + static void [[operator]] [[del^ete]](void *); + }; + )cpp", + R"cpp(// Overloaded operator spanning multiple tokens. + struct S { + void [[operator]] [[^(]][[)]](int); + }; + )cpp", }; for (const char *Test : Tests) { Annotations T(Test); >From 7491567917832fc2b42893e95488dd8623552b5e Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Fri, 11 Sep 2026 16:29:29 +0200 Subject: [PATCH 2/4] [clangd] Address review: extend the fix to explicit operator-call syntax, literal and conversion operators The previous commit only handled the operator's own declaration. Two review comments pointed out related gaps: - Explicit operator-call syntax (e.g. `a.operator+(b)`) still only highlighted `operator`, not the name that follows, because the code always pulled the name range from the *target declaration*, not from the referring expression. Generalize handleDeclOccurrence to read the DeclarationNameInfo off whichever AST node represents the current occurrence (MemberExpr, DeclRefExpr, and their dependent-context counterparts), falling back to the declaration only when the occurrence has none of its own (e.g. a `new`/`delete` expression, which references its operator without an expression of its own to carry a name). This incidentally still prevents reaching into another file's tokens, since a mismatch between the found name's location and the occurrence's own location now means we don't actually know how (or whether) the name is spelled here. - Literal operators (`operator""_x`) and conversion operators (`operator int()`) weren't covered at all, since neither is an "overloaded operator" in Clang's sense. Add both: for a literal operator, the suffix is lexed together with the preceding `""` as a single token, so the whole token is used; for a conversion operator, the target type's token range is used, which naturally handles arbitrarily complex types. Reviewers also noted that find-all-references/definition still only cover the `operator` keyword, and that clicking directly on a conversion operator's target type doesn't resolve to the declaration at all. Both are out of scope here: the former was already deliberately excluded from the previous commit (a merged multi-token reference can span multiple lines, which clients might not handle properly), and the latter is a pre-existing SelectionTree/targetDecl resolution gap unrelated to this code path. Assisted-by: Claude --- clang-tools-extra/clangd/XRefs.cpp | 92 ++++++++++++++++--- .../clangd/unittests/XRefsTests.cpp | 19 ++++ 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index bbaaf1305823c..5a15809957205 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -41,6 +41,7 @@ #include "clang/AST/StmtCXX.h" #include "clang/AST/StmtVisitor.h" #include "clang/AST/Type.h" +#include "clang/AST/TypeLoc.h" #include "clang/Basic/LLVM.h" #include "clang/Basic/Module.h" #include "clang/Basic/SourceLocation.h" @@ -1038,6 +1039,77 @@ tokensSpelledInRange(const syntax::TokenBuffer &TB, const SourceManager &SM, return Locs; } +/// If this occurrence is spelled as (part of) an "operator"-shaped name -- +/// `operator+`, `operator[]`, a literal operator like `operator""_x`, or a +/// conversion operator like `operator int()` -- returns the location of the +/// `operator` keyword followed by the locations of the tokens that make up +/// the rest of the name, so callers can highlight (or otherwise report) the +/// whole name instead of just the keyword. Returns an empty list otherwise, +/// including when \p Loc is some other occurrence of \p D (e.g. an implicit +/// operator call like `a + b`, which has no `operator` token to extend). +llvm::SmallVector<SourceLocation, 4> +operatorNameTokens(const Decl *D, + const index::IndexDataConsumer::ASTNodeInfo &ASTNode, + SourceLocation Loc, const syntax::TokenBuffer &TB, + const SourceManager &SM) { + std::optional<DeclarationNameInfo> NameInfo; + if (auto *ME = llvm::dyn_cast_or_null<MemberExpr>(ASTNode.OrigE)) + NameInfo = ME->getMemberNameInfo(); + else if (auto *DRE = llvm::dyn_cast_or_null<DeclRefExpr>(ASTNode.OrigE)) + NameInfo = DRE->getNameInfo(); + else if (auto *DSME = llvm::dyn_cast_or_null<CXXDependentScopeMemberExpr>( + ASTNode.OrigE)) + NameInfo = DSME->getMemberNameInfo(); + else if (auto *DSDRE = + llvm::dyn_cast_or_null<DependentScopeDeclRefExpr>(ASTNode.OrigE)) + NameInfo = DSDRE->getNameInfo(); + else if (auto *FD = llvm::dyn_cast_or_null<FunctionDecl>(D)) + NameInfo = FD->getNameInfo(); + // Not every occurrence carries its own name info. Most ways of invoking an + // operator without writing the `operator` keyword (e.g. `a + b`) still + // reference it through a real, if implicit, MemberExpr/DeclRefExpr callee + // that's handled by the cases above (and later filtered out below, since + // that implicit callee has no `operator` text to report). A `new T(...)` + // or `delete p;` *expression* is the odd one out: unlike a call, it has no + // callee sub-expression at all -- CXXNewExpr/CXXDeleteExpr just store the + // resolved FunctionDecl directly -- so indexing it passes no RefE, and we + // fall through to D's info above. There, NameInfo does not actually + // describe how the name is spelled at *this* occurrence -- it may belong + // to a distant reference, or even a declaration in another file entirely + // -- so its location won't match Loc. + if (!NameInfo || NameInfo->getLoc() != Loc) + return {}; + + SourceRange ExtraRange; + switch (NameInfo->getName().getNameKind()) { + case DeclarationName::CXXOperatorName: + ExtraRange = NameInfo->getCXXOperatorNameRange(); + break; + case DeclarationName::CXXLiteralOperatorName: { + // The suffix (e.g. `_test` in `operator""_test`) is lexed as part of a + // single string-literal-with-suffix token, so its location isn't a + // token's own start; look up the (whole) token that contains it. + SourceLocation SuffixLoc = NameInfo->getCXXLiteralOperatorNameLoc(); + if (SuffixLoc.isValid()) + if (const auto *Tok = TB.spelledTokenContaining(SM.getFileLoc(SuffixLoc))) + ExtraRange = SourceRange(Tok->location(), Tok->location()); + break; + } + case DeclarationName::CXXConversionFunctionName: + if (TypeSourceInfo *TInfo = NameInfo->getNamedTypeInfo()) + ExtraRange = TInfo->getTypeLoc().getSourceRange(); + break; + default: + break; + } + if (ExtraRange.getBegin().isInvalid()) + return {}; + + llvm::SmallVector<SourceLocation, 4> Result{Loc}; + llvm::append_range(Result, tokensSpelledInRange(TB, SM, ExtraRange)); + return Result; +} + /// Collects references to symbols within the main file. class ReferenceFinder : public index::IndexDataConsumer { public: @@ -1115,19 +1187,13 @@ class ReferenceFinder : public index::IndexDataConsumer { } else if (auto *OMD = llvm::dyn_cast_or_null<ObjCMethodDecl>(ASTNode.OrigD)) { OMD->getSelectorLocs(Locs); - } else if (auto *FD = llvm::dyn_cast_or_null<FunctionDecl>(D); - FD && FD->isOverloadedOperator() && - isInsideMainFile(FD->getNameInfo().getLoc(), SM)) { - // The operator name (e.g. `new`, `[]`, `<<`) is a separate token (or - // tokens) from the `operator` keyword itself; report both so the - // whole name gets highlighted, not just the keyword. Only do this - // when the declaration itself is in the main file: TB only has - // spelled tokens for the main file, and this is only useful anyway - // when we're looking at the occurrence at the declaration itself - // (checked below). - Locs.push_back(FD->getNameInfo().getLoc()); - auto OpNameRange = FD->getNameInfo().getCXXOperatorNameRange(); - llvm::append_range(Locs, tokensSpelledInRange(TB, SM, OpNameRange)); + } else { + // An "operator"-shaped name (operator+, operator""_x, operator + // int()) has a name that's a separate token (or tokens) from the + // `operator` keyword itself; report both so the whole name gets + // highlighted, not just the keyword. This covers both the + // declaration and explicit references to it, e.g. `a.operator+(b)`. + Locs = operatorNameTokens(D, ASTNode, Loc, TB, SM); } // Sanity check: we expect the *first* token to match the reported loc. // Otherwise, maybe it was e.g. some other kind of reference to a Decl. diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index 5641b716f552f..7f9ff8c0ecbf5 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -161,6 +161,25 @@ TEST(HighlightsTest, All) { void [[operator]] [[^(]][[)]](int); }; )cpp", + R"cpp(// Explicit operator-call syntax also highlights the whole name. + struct S { + S [[operator]] [[+]](S); + }; + void f(S a) { + a.[[operator]] [[^+]](a); + } + )cpp", + R"cpp(// Literal operator: the suffix is lexed together with the preceding + // `""` as a single token, so the whole thing is highlighted. + long double [[operator]] [[""_te^st]](long double); + )cpp", + R"cpp(// Conversion operator: the target type name is highlighted too. + // (Clicking on `int` itself doesn't resolve to the declaration at + // all, a separate limitation.) + struct S { + [[^operator]] [[int]](); + }; + )cpp", }; for (const char *Test : Tests) { Annotations T(Test); >From f3b9ce477ca35650ca5906c6b52c6b8897b04da4 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Mon, 14 Sep 2026 11:54:09 +0200 Subject: [PATCH 3/4] [clangd] Fix crash when an operator name comes from a macro expansion For instance: #define PLUS + struct S { void operator PLU^S(int); }; getCXXOperatorNameRange() for a macro-expanded operator symbol returns a macro location rather than a file location, which tokensSpelledInRange() didn't account for: it called SourceManager::getFileID()/getFileOffset() on it directly, eventually indexing into TokenBuffer's per-file token table with a FileID that doesn't correspond to any file it actually tracks. Reject non-file locations up front, matching the function's existing (but previously unenforced) documented precondition. This falls back to highlighting just `operator`, the same as before this whole feature was added, rather than trying to guess what, if anything, should be highlighted for a macro-expanded name. Assisted-by: Claude --- clang-tools-extra/clangd/XRefs.cpp | 10 +++++++--- clang-tools-extra/clangd/unittests/XRefsTests.cpp | 6 ++++++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index 5a15809957205..e92e42a102d77 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -1016,13 +1016,17 @@ std::vector<DocumentLink> getDocumentLinks(ParsedAST &AST) { namespace { /// Returns the locations of the spelled tokens overlapping [Range.getBegin(), -/// Range.getEnd()], in order. Both ends of \p Range must be file locations -/// in the same file. +/// Range.getEnd()], in order. Returns an empty list unless both ends of +/// \p Range are file locations in the same file: unlike a spelling or +/// expansion location, a macro location isn't something TokenBuffer (or the +/// FileID/offset arithmetic below) can make sense of, e.g. if part of an +/// operator name comes from a macro (`#define PLUS + ... operator PLUS(int)`). llvm::SmallVector<SourceLocation, 4> tokensSpelledInRange(const syntax::TokenBuffer &TB, const SourceManager &SM, SourceRange Range) { llvm::SmallVector<SourceLocation, 4> Locs; - if (Range.getBegin().isInvalid() || Range.getEnd().isInvalid()) + if (Range.getBegin().isInvalid() || Range.getEnd().isInvalid() || + !Range.getBegin().isFileID() || !Range.getEnd().isFileID()) return Locs; FileID FID = SM.getFileID(Range.getBegin()); if (FID != SM.getFileID(Range.getEnd())) diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index 7f9ff8c0ecbf5..52612e7c29762 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -180,6 +180,12 @@ TEST(HighlightsTest, All) { [[^operator]] [[int]](); }; )cpp", + R"cpp(// Regression: an operator name coming from a macro expansion must not + // crash. Since a macro location isn't something we can safely treat + // as spelled tokens, we fall back to highlighting just `operator`. + #define PLUS + + struct S { void [[operator]] PLU^S(int); }; + )cpp", }; for (const char *Test : Tests) { Annotations T(Test); >From cc8323fa08c379958a682167dbc13336412a59b2 Mon Sep 17 00:00:00 2001 From: Christian Kandeler <[email protected]> Date: Tue, 15 Sep 2026 10:28:38 +0200 Subject: [PATCH 4/4] [clangd] Highlight the whole name for a dependent overloaded-operator call Given struct S { void operator+(int); void operator+(double); }; template <typename T> void foo(S s, T t) { s.operator+(t); } clicking the `double` overload's declaration highlighted `operator` at the call site inside foo(), but not `+` -- unlike a non-overloaded operator+, where both got highlighted. The call's argument type is dependent, so overload resolution can't happen until instantiation: `s.operator+(t)`'s callee is an UnresolvedMemberExpr, not a plain MemberExpr. Its own name info wasn't among the cases operatorNameTokens() checked, so it fell through to the declaration's name info instead, which doesn't describe this occurrence (a different location entirely) and got correctly rejected -- just not usefully so, since the whole point was to extend from *this* occurrence's own name. UnresolvedMemberExpr and UnresolvedLookupExpr (its non-member counterpart, e.g. an ADL/dependent call to a free operator+) both derive from OverloadExpr, which carries the same real, spelled DeclarationNameInfo as the other expression kinds already handled. Add it as another case. Assisted-by: Claude --- clang-tools-extra/clangd/XRefs.cpp | 5 +++++ clang-tools-extra/clangd/unittests/XRefsTests.cpp | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index e92e42a102d77..e68e6a08adce5 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -1067,6 +1067,11 @@ operatorNameTokens(const Decl *D, else if (auto *DSDRE = llvm::dyn_cast_or_null<DependentScopeDeclRefExpr>(ASTNode.OrigE)) NameInfo = DSDRE->getNameInfo(); + else if (auto *OE = llvm::dyn_cast_or_null<OverloadExpr>(ASTNode.OrigE)) + // An UnresolvedMemberExpr/UnresolvedLookupExpr: overload resolution for + // this call is dependent (e.g. on a template parameter), so it reports a + // reference to every candidate at the same (real, spelled) location. + NameInfo = OE->getNameInfo(); else if (auto *FD = llvm::dyn_cast_or_null<FunctionDecl>(D)) NameInfo = FD->getNameInfo(); // Not every occurrence carries its own name info. Most ways of invoking an diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp index 52612e7c29762..9d1a34f23955c 100644 --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/clang-tools-extra/clangd/unittests/XRefsTests.cpp @@ -186,6 +186,19 @@ TEST(HighlightsTest, All) { #define PLUS + struct S { void [[operator]] PLU^S(int); }; )cpp", + R"cpp(// Regression: an overloaded operator called with dependent arguments + // (so overload resolution is deferred, producing an + // UnresolvedMemberExpr with several candidates at one location) + // should still have its whole name highlighted, not just `operator`. + struct S { + void operator+(int); + void [[operat^or]] [[+]](double); + }; + template <typename T> + void foo(S s, T t) { + s.[[operator]] [[+]](t); + } + )cpp", }; for (const char *Test : Tests) { Annotations T(Test); _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
