[PATCH] D107137: clangd: Provide hover info for include directives

Fri Jul 30 05:32:10 PDT 2021

sammccall added a comment.

Thanks, this is great! I think we need to hold the line a bit on keeping different hovers uniform though.

================
Comment at: clang-tools-extra/clangd/Hover.cpp:914
+          HoverInfo HI;
+          HI.TargetFile = File->Definition;
+          return HI;
----------------
A HoverInfo with no name and no kind is a bit of a problem.

For the markdown rendering you're bailing out with a special case, which works but makes the layout inconsistent with other hovers. It's also harder to reason about the code when many kinds of symbols take different codepaths, which is one of the reasons we'd unified it.
We also have other clients of the C++ API that rely on name and preferably kind being present.

What about making `Name` the basename e.g. `foo.h` for `#include "project/foo.h"`

For Kind, there's no perfect Kind (using Module seems too confusing) and we can't easily extend the enum since we don't own it (we should fix this sometime...). Maybe just a FIXME there.

For the full path, it would be more consistent to store this in Definition if we can.
Only problem is Definition is a code block syntax-highlighted as C++. Quoting it will cause problems on windows (ugly escaped backslashes).
So I'd suggest adding `const char* DefinitionLanguage = "cpp"` to HoverInfo, and overriding it here. (And passing it through to codeBlock in present()).

================
Comment at: clang-tools-extra/clangd/XRefs.cpp:537

+llvm::Optional<LocatedSymbol> locateFileReferent(const Position &Pos,
+                                                 ParsedAST &AST,
----------------
This function is pretty simple and we don't use most of the result for hover.
Rather than making it public (and testing it!) and depending from hover->xrefs, can we just copy the needed bits into hover.cpp?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107137