[PATCH] D114522: [clangd] Add desugared type to hover

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thank you! (& Sorry for the delay, have been sick)
LG with the understanding we may have to iterate on behavior/configurability.

As noted below I'd suggest either disabling aka for params, or reverting to the previous version, but I don't feel strongly.



================
Comment at: clang-tools-extra/clangd/Hover.cpp:1255
   if (P.Type)
-    Output.push_back(*P.Type);
+    OS << llvm::to_string(*P.Type);
   if (P.Name)
----------------
this can just be << *P.Type, skipping the temporary string


================
Comment at: clang-tools-extra/clangd/Hover.cpp:1271
     OS << " = " << *P.Default;
+  if (P.Type && P.Type->AKA)
+    OS << llvm::formatv(" (aka {0})", *P.Type->AKA);
----------------
kadircet wrote:
> sammccall wrote:
> > Hmm, this seems to render as:
> > 
> > ```std::string Foo = "" (aka basic_string<char>)```
> > 
> > it's not *completely* clear what the aka refers to.
> > I don't have a better solution, though.
> > @kadircet any opinions here?
> i think it's attached to the type of the parameter, not it's name nor the initializer-expr. so I think we should move this closer to the type (i.e. `std::string (aka basic_sting<char>) Foo = ""`, basically `llvm::toString(P.Type)` ?)
> i think it's attached to the type of the parameter

This is logically correct but I think it's harder to read. This puts text in the middle of code, in a way that doesn't seem obvious to me: parens mean several things in C++ and it may be hard to recognize this means none of them.

Worst case is we have function types: `word(*)() (aka long(*)()) x = nullptr`

It also disrupts the reading flow in the case where the aka is *not* needed for understanding.
I think overall the previous version was better, though not great.

I'm tempted to say we should scope down this patch further until we have a better feel for how it behaves, i.e. exclude param types from aka for now. Param types are less obviously useful to disambiguate than result types. (e.g. because in most cases you can hover over the input expression).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114522