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

[xndcn via Phabricator via cfe-commits]

xndcn added inline comments.


================
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();
----------------
kadircet wrote:
> what about using the existing `getHoverContents(QualType ..)` overload instead ?
> what about using the existing `getHoverContents(QualType ..)` overload instead ?




================
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();
----------------
xndcn wrote:
> kadircet wrote:
> > what about using the existing `getHoverContents(QualType ..)` overload instead ?
> > what about using the existing `getHoverContents(QualType ..)` overload instead ?
> 
> 
Thanks, I tried using `getHoverContents(QualType ..)` overload and the result looks more simplicity:
{F14312230}

The origin patch HoverInfo looks like:
{F14312245}

Both seems reasonable, not sure which one is better.


================
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
----------------
kadircet wrote:
> can you handle `CXXThisExpr` inside this function, instead of an extra branch in the `getHover`?
Thanks, here need additional parameter `Index *`, will update patch soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92041