sammccall added a comment.
A few comments on the behavior & clangd changes. I can't say I understand
enough about how deduction is implemented to give a proper review of the guts
:-(
In general eliminating type-parameter-0-0 is great and retaining sugar gives us
more options for display.
Thanks for updating clangd, I have some quibbles but feel free to land this in
~any form where the tests pass, there are no critical regressions and I'm happy
to polish the behavior afterwards.
If I understand, a deduced decltype(auto) now desugars as `decltype(auto) ->
decltype(foo) -> Foo`.
This seems principled, but at least clangd we usually call getDeducedType() to
display the underlying type somehow, and `decltype(foo)` isn't terribly
enlightening. (The asymmetry between `auto` and `decltype(auto)` is a bit
unfortunate).
For clangd, unwrapping it in the `clangd::getDeducedType()` helper seems fine,
so I only really mention this because I get the same feeling about some of the
clang diagnostic messages.
It might be that the DeducedAsType for `decltype(auto)` should skip the
DeclType (when possible, i.e. unless dependent), WDYT?
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:495
+ const QualType &Type,
+ bool Underlying = false) {
const auto &SM = AST.getSourceManager();
----------------
(i'm confused by the optional parameter - there's only one caller)
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:506
+ targetDecl(DynTypedNode::create(Type.getNonReferenceType()),
+ DeclRelation::TemplatePattern | DeclRelation::Alias |
+ (Underlying ? DeclRelation::Underlying : DeclRelation()),
----------------
`Alias | Underlying` means that go-to-definition will return both A and B in
this case:
```
using A = int;
using B = A;
au^to x = B();
```
I don't think we particularly want that.
Really this change is just trying to make sure we can resolve `decltype(auto)
-> decltype(foo) -> Foo` right? I think we should rather make
clangd::getDeducedType(...) unwrap the decltype(foo) in the first place.
================
Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:449
)cpp",
- ExpectedHint{": int &", "z"});
+ ExpectedHint{": decltype(y)", "z"});
}
----------------
this is a regression, again retaining sugar is good in general but we need to
unwrap the deduced decltype.
(No need to fix this, I'm happy to do it as a followup)
================
Comment at:
clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p7-cxx14.cpp:69
auto multi1c = multi1a, multi1d = multi1b;
-decltype(auto) multi1e = multi1a, multi1f = multi1b; // expected-error
{{'decltype(auto)' deduced as 'int' in declaration of 'multi1e' and deduced as
'int &' in declaration of 'multi1f'}}
+decltype(auto) multi1e = multi1a, multi1f = multi1b; // expected-error
{{'decltype(auto)' deduced as 'decltype(multi1a)' (aka 'int') in declaration of
'multi1e' and deduced as 'decltype(multi1b)' (aka 'int &') in declaration of
'multi1f'}}
----------------
here's an example where I think retaining the `decltype(expr)` sugar probably
adds more noise than insight
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110216/new/
https://reviews.llvm.org/D110216
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
