[PATCH] D92041: Add hover info for `this` expr

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Thanks, this sounds like a sensible idea. I got a few suggestions for the implementation though.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:610
+/// Generate a \p Hover object given the \p this pointer.
+HoverInfo getHoverContents(const CXXThisExpr *CTE, const SymbolIndex *Index) {
+  const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();
----------------
what about using the existing `getHoverContents(QualType ..)` overload instead ?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:644
 // FIXME: Support hover for literals (esp user-defined)
 llvm::Optional<HoverInfo> getHoverContents(const Expr *E, ParsedAST &AST) {
   // There's not much value in hovering over "42" and getting a hover card
----------------
can you handle `CXXThisExpr` inside this function, instead of an extra branch in the `getHover`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92041