[PATCH] D133979: [clangd] XRef functions treat visible class definitions as primary.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:291
+      // (This exception matches the spirit of our indexing heuristics).
+      if (/*PreferDef=*/[&] {
+        if (Def == TD)
----------------
can we extract all of this into a `const TagDecl* getPublicDecl(const TagDecl* TD)` ?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:303
+        // decl is some other header.
+        return isHeaderFile(SM.getFileEntryRefForID(DefFile)->getName(),
+                            D->getASTContext().getLangOpts());
----------------
nit: we actually have the main file name in ParsedAST, but it's probably not worth propagating that into here.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:303
+        // decl is some other header.
+        return isHeaderFile(SM.getFileEntryRefForID(DefFile)->getName(),
+                            D->getASTContext().getLangOpts());
----------------
kadircet wrote:
> nit: we actually have the main file name in ParsedAST, but it's probably not worth propagating that into here.
reaching here actually implies user is in the same file that has the type definition (I know we were mostly discussing this for index behaviour, and our reasoning for the index makes completely sense) but I am not sure if taking the user to public declaration, when they're actually writing some code in the same file as the type's private implementation is still the right thing to do.

I think; if the definition is visible through AST, we should always prefer that, unless it's in a private header (which in theory we can detect here, as we've information about include graph).
do you think jumping to public interface is still desirable even when the user is already implementing something inside the private implementation file?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133979/new/

https://reviews.llvm.org/D133979