[PATCH] D111039: [clangd] Include refs of base method in refs for derived method.

Haojian Wu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 4 05:29:35 PDT 2021


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



More information about the cfe-commits mailing list