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

[liu hui via Phabricator via cfe-commits]

lh123 added inline comments.


================
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);
----------------
sammccall wrote:
> 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).
> 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).

I‘d like to keep the `a.k.a` type for parameter. because: 
1. sometime we pass `literal (or null pointer)` as parameter, but clang doesn't support hover on literal. eg:
```
using mint = int;
void foo(mint *);
void code() {
    foo(nullptr); // hover over foo, we can get 'mint * (aka int *)'
}
```
2. It may be useful when making function calls, although it does not work well when the function is overloaded.**(add a.k.a type to signature help?)** eg:
```
using mint = int;
void foo(mint *);
void code() {
    foo(); // hover on foo, we can get 'mint * (aka int *)'
}
```

>  I think overall the previous version was better, though not great.

Agree with that, it seems that putting a.k.a in the middle of the code makes the hover look bad based on my recent use


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114522