hokein added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1395
if (CMD->isVirtual())
- if (IdentifierAtCursor && SM.getSpellingLoc(CMD->getLocation()) ==
- IdentifierAtCursor->location()) {
----------------
kadircet wrote:
> it's important to note that this doesn't only suppress xrefs behaviour
> outside of the declaration, but it also suppresses xrefs on parts of the
> declaration apart from the function's name (like override/virtual keywords).
> it's unfortunate that we don't have tests to explicitly point out that fact.
> I am not sure why the decision was to exclude them in the first place, I am
> definitely in favor of the new behaviour (we already provide go-to-def on
> `override` keyword for example, not having xrefs on it seems surprising). I
> suppose @hokein can tell if there are any other concerns I am missing.
yeah, I think the initial design was to return all base-class references when
xrefs is called at the *declaration* (our previous concern was that base-class
refs might add a lot of noises to the final results, which might be not what
users want). Now we have user complains, let's do it for all places (LG the
change).
Re the behavior on override keyword, this is probably an oversight (xrefs was
not considered when we extended go-to-def). The implementation of go-to-def is
ad-hoc, and only works for `override`, and `final` keywords (due to the
limitation of AST). not sure it is a really useful feature, for consistency,
it would be nice to have it in xrefs. I think it can be addressed in a
follow-up patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111039/new/
https://reviews.llvm.org/D111039
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
