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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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();
----------------
xndcn wrote:
> kadircet wrote:
> > xndcn wrote:
> > > 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.
> > I am not sure if there's much value in repeating `this` and `type definition`. So I would go with using the `QualType` overload here.
> > But, rather than providing `CTE->getType()` directly, i believe we should display information for the `PointeeType`, as it will also display comments and such for `TagDecls`, rather than just providing the type-name.
> Thanks! It looks more simplicity with `PointeeType`. Shall we keep the additional information like namespace and template parameters? So we can use `getHoverInfo(const NamedDecl ..)` overload?
> {F14313888}
i was thinking those would be just repeating what's already available but you seem to be right. especially namespace and template arguments might be useful.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:631
 
 // Generates hover info for evaluatable expressions.
 // FIXME: Support hover for literals (esp user-defined)
----------------
let's update the comment too. "Generates hover info for `this` and evaluatable expressions."


================
Comment at: clang-tools-extra/clangd/Hover.cpp:651
     return HI;
+  } else if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E)) {
+    const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();
----------------
can you put this above `printExprValue`, also the else is unnecessary as branch ends with return, i.e:

```
if(thisexpr) {
...
return ..;
}
if(printExprValue..) {
...
return ..;
}
```


================
Comment at: clang-tools-extra/clangd/Hover.cpp:652
+  } else if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E)) {
+    const NamedDecl *D = CTE->getType()->getPointeeType()->getAsCXXRecordDecl();
+    HI = getHoverContents(D, Index);
----------------
s/getAsCXXRecordDecl/getAsTagDecl/


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2023
+      {
+          R"cpp(// this expr
+           namespace ns {
----------------
can you add two more test cases, one for a template class and one for a specialization:

```
template <typename T> 
struct Foo {
  Foo* bar() { return [[thi^s]]; }
};
template <> 
struct Foo<int> {
  Foo* bar() { return [[thi^s]]; }
};
```


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

https://reviews.llvm.org/D92041