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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
