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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for the delay here. Kadir is out on vacation.

Yikes - it's a shame reusing our existing type printing doesn't do the right thing, but injected-classname and partial specializations are indeed weird.
I'm tempted to say just to live with the "type-parameter-0-0" nonsense rather than implement the workaround, but it's up to you.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:640
   HoverInfo HI;
+  // For `this` expr we currently generate hover with class declaration.
+  if (const CXXThisExpr *CTE = dyn_cast<CXXThisExpr>(E)) {
----------------
this needs a comment explaining why we can't reuse existing logic... as far as that can be explained.

The body here is complicated, so should probably be factored out into another function.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:643
+    std::string Name;
+    llvm::raw_string_ostream OS(Name);
+
----------------
are we missing CV qualifiers?


================
Comment at: clang-tools-extra/clangd/Hover.cpp:646
+    const NamedDecl *ND = CTE->getType()->getPointeeType()->getAsTagDecl();
+    const auto NS = getNamespaceScope(ND);
+    if (!NS.empty()) {
----------------
getNamespaceScope isn't going to do the right thing for classes nested in classes.

However I think we *don't* want to print the scope here anyway.

Generally we just put the name in the hover. I get the argument for following `auto`, but `auto` is basically a weird historical exception (it was something like the first hover supported).
And it's clear that you need namespace qualifiers for auto (it could be anything!) but less clear you need it here as we're guaranteed to be inside the type. I'd suggest just dropping it.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:666
+        if (P.Default)
+          OS << " = " << P.Default;
+      }
----------------
why are we including default template param values? these are not part of the type.


================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2023
+      {
+          R"cpp(// this expr
+           namespace ns {
----------------
kadircet wrote:
> 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]]; }
> };
> ```
And a test for const?


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

https://reviews.llvm.org/D92041