qchateau added inline comments.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:618
+ } else {
+ CXXRecordDecl *D = QT->getAsCXXRecordDecl();
+ if (D && D->isLambda())
----------------
sammccall wrote:
> You've rewritten this logic compared to the old `getHoverContents(QualType)`,
> and I think there are regressions:
> - We've dropped class documentation (see e.g.
> ac3f9e48421712168884d59cbfe8b294dd76a19b, this is visible in the tests)
> - we're no longer using printName() to print tag-decls, which I expect
> changes e.g. the printing of anyonymous structs which is special-cased in
> that function (we have tests for this but probably not in combination with
> auto)
> - we're no longer using the printing-policy to print non-tag types, which
> will lead to some random changes
>
> I don't see a reason for these changes, can we revert them?
- I can re-add class documentation, but should it work when `auto` is a pointer
or a ref ? In that case, I'll need something like your `unwrapType` of
https://reviews.llvm.org/D93314
- `printName` will print `C` instead of `class C` even if I hack around and
give it the right `PrintingPolicy`. The problem is that IDE (at least VSCode)
does not know what `C` is and cannot color it, which looks a nice less nice. Up
to you !
- I can re-add the printing policy for non-tag types, and add it for tag-types
if we chose not to use `printName`.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:631
+// getDeducedType are handled.
+class ExtraAutoTypeHoverVisitor
+ : public RecursiveASTVisitor<ExtraAutoTypeHoverVisitor> {
----------------
sammccall wrote:
> This functionality (reporting the `auto` type in structured bindings, and not
> reporting non-deduced uses of auto) belongs in the getDeducedType() helper
> rather than as a layer on top.
> (Especially because it has to be done via an AST traversal rather than
> SelectionTree)
>
> I'd suggest leaving it out of this patch to get this the original change
> landed quickly - this seems like a mostly-unrelated enhancement. But up to
> you.
I'll remove this. I'll leave out the case of structured bindings for arrays
(which is really weird and specific), but I guess I'll fix`getDeducedType` to
return the undeduced `QualType` instead of `None` (which it already does but
only for return types).
================
Comment at: clang-tools-extra/clangd/Hover.cpp:907
+
+ HI->Name = tok::getTokenName(Tok.kind());
+ HighlightRange = Tok.range(SM).toCharRange(SM);
----------------
sammccall wrote:
> decltype(auto) and decltype(expr) are fairly different things and ultimately
> we should be displaying them differently I think ("decltype(auto)" vs
> "decltype(...)").
>
> Unfortunately it's awkward because our getDeducedType helper handles both at
> the moment (and so is misnamed, because decltype(expr) isn't deduced at all).
>
> Can you add `// FIXME: distinguish decltype(auto) vs decltype(expr)` and I'll
> do some refactoring later?
Sure, I'll add the comment. I'll leave that refactoring to you, I'm not quite
sure how you intent to achieve it.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:999
+ "decltype(au^to) x = 0;",
+ R"cpp(// Lambda auto parameter. Nothing (Not useful).
+ auto lamb = [](a^uto){};
----------------
sammccall wrote:
> (not convinced this is fundamentally not useful - the fact that it's a
> template parameter means it's probably worth having a hover card for it at
> some point. But I agree with suppressing it for now)
As a user I'd prefer the hover to work over the whole `decltype(auto)`
expression. But that does not seem quite compatible with the way tokens are
parsed.
Are you suggesting I remove the test case or should I add a `FIXME` comment ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93227/new/
https://reviews.llvm.org/D93227
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
