[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Fix pushed to https://reviews.llvm.org/D114207


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3139129 , @lkail wrote:

> Hi we found regression in our internal tests. It compiles with clang-13.0.0 
> https://godbolt.org/z/3abGrcf7o and gcc https://godbolt.org/z/K9oj3Grs1, but 
> fails with clang-trunk https://godbolt.org/z/1Tehxa1x9. Is it an intended 
> change? If so, do we have to document this?

Thanks for the report!

No it was not intended change, this patch should not affect partial ordering 
like that, modulo any bugs we might have accidentally fixed.
What happened here is that we lost a very small piece of code in the 
refactoring, which was introduced more than 10 years ago, but had no test 
coverage.
You just provided it though, and I am working on a patch to restore it shortly.

In D110216#3140678 , @thakis wrote:

> Haha :) Thanks for taking a look. aka for typedefs sounds like a nice 
> approach to me.

Thanks, low on bandwidth right now but I will get to it :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

> So, some notes:
>
> - We should probably also do an AKA on the type diff.
> - Chromium should use a clearer name for that typedef perhaps? Though calling 
> the first template parameter of span as `T` is common, maybe a better name 
> here would be `element_type`.
>
> Having a convenient way to just name a type like that also seems useful, so 
> maybe the rename coupled with the AKA would be an improvement overall?

Haha :) Thanks for taking a look. aka for typedefs sounds like a nice approach 
to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Kai Luo via Phabricator via cfe-commits]

lkail added a comment.

Hi we found regression in our internal tests. It compiles with clang-13.0.0 
https://godbolt.org/z/3abGrcf7o and gcc https://godbolt.org/z/K9oj3Grs1, but 
fails with clang-trunk https://godbolt.org/z/1Tehxa1x9. Is it an intended 
change? If so, do we have to document this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Isolated example:

  template  struct span {};
  
  auto make_span() {
using T = int;
return span();
  }
  
  void WontCompile() {
span s1 = make_span();
  }

https://godbolt.org/z/rjd6Y6f9d

  testcc:9:13: error: no viable conversion from 'span' to 
'span'
span s1 = make_span();
  ^~~~
  test.cc:1:34: note: candidate constructor (the implicit copy constructor) not 
viable: no known conversion from 'span'
(aka 'span') to 'const span &' for 1st argument
  template  struct span {};
   ^
  test.cc:1:34: note: candidate constructor (the implicit move constructor) not 
viable: no known conversion from 'span'
(aka 'span') to 'span &&' for 1st argument
  template  struct span {};


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3137375 , @hans wrote:

>> I am not sure how to reproduce this.
>
> Attaching preprocessed source. With Clang built at 
> 508aa4fe0c82b3b409e2e194d591ee6d665c8623 it reproduces for me like this:
>
>   $ clang++ -c /tmp/a.ii
>   ../../base/containers/span_unittest.cc:11:13: error: no viable conversion 
> from 'span' to 'span'
> span s = make_span(v);
>   ^   
>   ../../base/containers/span.h:340:13: note: candidate constructor not 
> viable: no known conversion from 'span' (aka 'span int, Extent::value>') to 'const base::span &' for 
> 1st argument
> constexpr span(const span& other) noexcept = default;
>   ^

Oh so that one was a curve ball `T` was not a template parameter, just a 
typedef, see the implementation of make_span in that preprocessed file:

  template 
  constexpr auto make_span(Container&& container) noexcept {
using T =
std::remove_pointer_t()))>;
using Extent = internal::Extent;
return span(std::forward(container));
  }

So, some notes:

- We should probably also do an AKA on the type diff.
- Chromium should use a clearer name for that typedef perhaps? Though calling 
the first template parameter of span as `T` is common, maybe a better name here 
would be `element_type`.

Having a convenient way to just name a type like that also seems useful, so 
maybe the rename coupled with the AKA would be an improvement overall?

Otherwise, if we get too much trouble with with legacy codebases which cannot 
be updated with that simple rename, we could perhaps make the type printer be 
context sensitive, and when printing type nodes tied to NamedDecls, just step 
over those that refer to a name which is not accessible in the current context?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

Sorry, the attached file is here: F20424768: a.ii 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

> I am not sure how to reproduce this.

Attaching preprocessed source. With Clang built at 
508aa4fe0c82b3b409e2e194d591ee6d665c8623 it reproduces for me like this:

  $ clang++ -c /tmp/a.ii
  ../../base/containers/span_unittest.cc:11:13: error: no viable conversion 
from 'span' to 'span'
span s = make_span(v);
  ^   
  ../../base/containers/span.h:340:13: note: candidate constructor not viable: 
no known conversion from 'span' (aka 'span') to 'const base::span &' for 1st 
argument
constexpr span(const span& other) noexcept = default;
  ^
  ../../base/containers/span.h:303:13: note: candidate template ignored: could 
not match 'int[N]' against 'span' (aka 'span')
constexpr span(T ()[N]) noexcept : span(base::data(array), N) {}
  ^
  ../../base/containers/span.h:310:13: note: candidate template ignored: could 
not match 'array' against 'span'
constexpr span(std::array& array) noexcept
  ^
  ../../base/containers/span.h:317:13: note: candidate template ignored: could 
not match 'array' against 'span'
constexpr span(const std::array& array) noexcept
  ^
  ../../base/containers/span.h:328:13: note: candidate template ignored: 
requirement 
'conjunction>>, 
base::negation>>, base::negation>>, std::is_convertible, 
std::is_integral>::value' was not satisfied [with Container = 
base::span]
constexpr span(Container& container) noexcept{F20424748}
  ^
  ../../base/containers/span.h:337:13: note: candidate template ignored: 
requirement 
'conjunction>>, 
base::negation>>, base::negation>>, std::is_convertible, 
std::is_integral>::value' was not satisfied [with Container = 
base::span]
constexpr span(const Container& container) noexcept
  ^
  ../../base/containers/span.h:350:13: note: candidate template ignored: 
requirement 'is_convertible::value' was not 
satisfied [with U = const int, OtherExtent = 18446744073709551615]
constexpr span(const span& other)
  ^
  1 error generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3135461 , @hans wrote:

> We're seeing a diagnostic change in Chromium which looks funny. For the 
> following code (span<> is our own class):
> 
> the diagnostic changes from:
>
>   fatal error: no viable conversion from 'span' to 
> 'span
>
> to
>
>   fatal error: no viable conversion from 'span' to 'span
>
> This looks very strange.

Hi! thanks for the report.

I am not sure how to reproduce this.

I tried mocking this situation:

  template  struct vector {};
  template  struct span {};
  
  template  auto make_span(const vector &) { return span(); }
  
  void WontCompile() {
const vector v;
  
span s1 = make_span(v);
  }

Which I get:

  error: no viable conversion from 'span' to 'span'

Looking at the `dump` of that type:

  AutoType 0x246b83676c0 'span' sugar
  `-ElaboratedType 0x246b8366d60 'span' sugar
`-TemplateSpecializationType 0x246b8366d20 'span' sugar span
  |-TemplateArgument type 'const int':'const int'
  | `-QualType 0x246b8366751 'const int' const
  |   `-SubstTemplateTypeParmType 0x246b8366750 'int' sugar
  | |-TemplateTypeParmType 0x246b835c790 'T' dependent depth 0 index 0
  | | `-TemplateTypeParm 0x246b835c740 'T'
  | `-BuiltinType 0x246b82e81e0 'int'
  `-RecordType 0x246b8366d00 'struct span'
`-ClassTemplateSpecialization 0x246b8366be8 'span'

In particular, the `SubstTemplateTypeParmType` desugars to int, not `T`, and I 
am not sure how you managed to get that ellipsis (from the type diff) on the 
template specialization arguments.

I am looking though at a clang built from `D112374`, which is main with what I 
believe are other patches that have no effect here besides the addition of the 
`ElaboratedType` node on that dump.

To be sure, it's going to take me a while to get a main built here, so 
hopefully this is not a big problem for you right now?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

We're seeing a diagnostic change in Chromium which looks funny. For the 
following code (span<> is our own class):

  int WontCompile() {
const std::vector v;
span s = make_span(v);
  }

the diagnostic changes from:

  fatal error: no viable conversion from 'span' to 'span

to

  fatal error: no viable conversion from 'span' to 'span

This looks very strange.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Louis Dionne via Phabricator via cfe-commits]

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

LGTM from the libc++ perspective.

Unsubscribing to reduce spam, please ping me on Discord if you need further 
input. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

FYI this is a minimal repro taken from Martin's preprocessed source (Thanks!):

  template  struct a { using b = const float; };
  template  using d = typename a::b;  
  
  template  void e(d *, c) {}
  template void e(typename a::b *, int);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov reopened this revision.
mizvekov added a comment.

Reverting for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Robert Richmond via Phabricator via cfe-commits]

RobRich999 added a comment.

Hit libANGLE in Chromium today, too.

https://bugs.chromium.org/p/chromium/issues/detail?id=1270154


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3130184 , @mstorsjo wrote:

> Do you happen to know what's going on here? The files mentioned are 
> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp?h=5.15.1
>  and 
> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h?h=5.15.1.

Thanks for the report!

Not immediately obvious to me, would need some time to isolate / create a 
minimal test case.
Would you be in a position to extract a preprocessed source file + command line 
which repros this error?

Also, feel free to revert if it helps you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D110216#3130193 , @mizvekov wrote:

> In D110216#3130184 , @mstorsjo 
> wrote:
>
>> Do you happen to know what's going on here? The files mentioned are 
>> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp?h=5.15.1
>>  and 
>> https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h?h=5.15.1.
>
> Thanks for the report!
>
> Not immediately obvious to me, would need some time to isolate / create a 
> minimal test case.
> Would you be in a position to extract a preprocessed source file + command 
> line which repros this error?

Sure: https://martin.st/temp/angle-preproc.cpp, built with `clang -target 
x86_64-w64-mingw32 -std=c++17 -w -c angle-preproc.cpp`.

> Also, feel free to revert if it helps you.

No big hurry for me wrt that yet, at least until we know which part is at fault 
here.

(I haven't tested with the very latest Qt and/or ANGLE - the full build setup 
for this is rather complex.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

This broke building ANGLE as part of Qt 5.15 for a mingw target, with the 
following error:

  
../../../3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp:532:38:
 error: explicit instantiation of 'allocate' does not refer to a function 
template, variable template, member function, member class, or static data 
member
  ANGLE_RESOURCE_TYPE_OP(Instantitate, ANGLE_INSTANTIATE_OP)
   ^
  
../../../3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h:301:15:
 note: candidate template ignored: could not match 'GetInitDataFromD3D11' 
(aka 'typename 
InitDataType::Value>::Value') against 
'const D3D11_SUBRESOURCE_DATA' 
  gl::Error allocate(Renderer11 *renderer,
^
  
../../../3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp:358:30:
 note: candidate template ignored: could not match 'GetInitDataFromD3D11' 
(aka 'typename 
InitDataType::Value>::Value') against 
'const D3D11_SUBRESOURCE_DATA'
  gl::Error ResourceManager11::allocate(Renderer11 *renderer,
   ^

Do you happen to know what's going on here? The files mentioned are 
https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.cpp?h=5.15.1
 and 
https://code.qt.io/cgit/qt/qtbase.git/tree/src/3rdparty/angle/src/libANGLE/renderer/d3d/d3d11/ResourceManager11.h?h=5.15.1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

Okay, libcxx pipeline passes, disregarding the clang-format failure for 
pre-existing badness in those files, which would need to be fixed separately or 
else git loses track of the rename.
The AIX failures also seem completely unrelated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

I think this should do it:

  diff --git 
a/libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.fail.cpp
 
b/libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.verify.cpp
  similarity index 81%
  rename from 
libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.fail.cpp
  rename to 
libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.verify.cpp
  index 234efc83423b..a28e3fc9d1d7 100644
  --- 
a/libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.fail.cpp
  +++ 
b/libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.verify.cpp
  @@ -18,6 +18,13 @@
   
   // Try and cast away const.
   
  +// This test only checks that we static_assert in any_cast when the
  +// constraints are not respected, however Clang will sometimes emit
  +// additional errors while trying to instantiate the rest of any_cast
  +// following the static_assert. We ignore unexpected errors in
  +// clang-verify to make the test more robust to changes in Clang.
  +// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
  +
   #include 
   
   struct TestType {};
  @@ -30,19 +37,15 @@ int main(int, char**)
   
   any a;
   
  -// expected-error@any:* {{drops 'const' qualifier}}
   // expected-error-re@any:* {{static_assert failed{{.*}} "ValueType is 
required to be a const lvalue reference or a CopyConstructible type"}}
   any_cast(static_cast(a)); // expected-note 
{{requested here}}
   
  -// expected-error@any:* {{cannot cast from lvalue of type 'const 
TestType' to rvalue reference type 'TestType &&'; types are not compatible}}
   // expected-error-re@any:* {{static_assert failed{{.*}} "ValueType is 
required to be a const lvalue reference or a CopyConstructible type"}}
   any_cast(static_cast(a)); // expected-note 
{{requested here}}
   
  -// expected-error@any:* {{drops 'const' qualifier}}
   // expected-error-re@any:* {{static_assert failed{{.*}} "ValueType is 
required to be a const lvalue reference or a CopyConstructible type"}}
   any_cast(static_cast(a)); // expected-note 
{{requested here}}
   
  -// expected-error@any:* {{cannot cast from lvalue of type 'const 
TestType2' to rvalue reference type 'TestType2 &&'; types are not compatible}}
   // expected-error-re@any:* {{static_assert failed{{.*}} "ValueType is 
required to be a const lvalue reference or a CopyConstructible type"}}
   any_cast(static_cast(a)); // expected-note 
{{requested here}}
   
  diff --git 
a/libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.fail.cpp
 
b/libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.verify.cpp
  similarity index 81%
  rename from 
libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.fail.cpp
  rename to 
libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.verify.cpp
  index 44a67f7aa03d..ec40c11b 100644
  --- 
a/libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.fail.cpp
  +++ 
b/libcxx/test/std/utilities/any/any.nonmembers/any.cast/not_copy_constructible.verify.cpp
  @@ -24,6 +24,13 @@
   
   // Test instantiating the any_cast with a non-copyable type.
   
  +// This test only checks that we static_assert in any_cast when the
  +// constraints are not respected, however Clang will sometimes emit
  +// additional errors while trying to instantiate the rest of any_cast
  +// following the static_assert. We ignore unexpected errors in
  +// clang-verify to make the test more robust to changes in Clang.
  +// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
  +
   #include 
   
   using std::any;
  @@ -45,17 +52,14 @@ struct no_move {
   int main(int, char**) {
   any a;
   // expected-error-re@any:* {{static_assert failed{{.*}} "ValueType is 
required to be an lvalue reference or a CopyConstructible type"}}
  -// expected-error@any:* {{static_cast from 'no_copy' to 'no_copy' uses 
deleted function}}
   any_cast(static_cast(a)); // expected-note {{requested 
here}}
   
   // expected-error-re@any:* {{static_assert failed{{.*}} "ValueType is 
required to be a const lvalue reference or a CopyConstructible type"}}
  -// expected-error@any:* {{static_cast from 'const no_copy' to 'no_copy' 
uses deleted function}}
   any_cast(static_cast(a)); // expected-note 
{{requested here}}
   
   any_cast(static_cast(a)); // OK
   
   // expected-error-re@any:* {{static_assert failed{{.*}} "ValueType is 
required to be an rvalue reference or a CopyConstructible type"}}
  -// expected-error@any:* {{static_cast from 'typename 
remove_reference::type' (aka 'no_move') to 'no_move' uses deleted 
function}}
   any_cast(static_cast(a));
   
 return 0;

Use `git apply` -- in particular mind the renaming of the files.


Repository:
  rG LLVM Github Monorepo

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D110216#3127530 , @mizvekov wrote:

> Exactly what Aaron said :-)
>
> Though might be a complication here if libcxx tests this with both ToT and 
> stable clang, so could be perhaps this needs to be changed in a way that 
> works in both.
>
> I will check this much later today, having a busy day.
>
> Adding Louis for the libcxx expertise.

Thanks for the heads up, I'm thinking about a fix right now. I'll paste the 
diff here and you can incorporate it to this review, which should trigger 
libc++ CI and make sure everything works.

We should probably start running the libc++ CI on Clang changes systematically, 
but I'm somewhat concerned about the bandwidth of our CI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a subscriber: ldionne.
mizvekov added a comment.

Exactly what Aaron said :-)

Though might be a complication here if libcxx tests this with both ToT and 
stable clang, so could be perhaps this needs to be changed in a way that works 
in both.

I will check this much later today, having a busy day.

Adding Louis for the libcxx expertise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Obviously the error message was just extended by the as-written type. Could 
have just adapted the expected results instead of reverting...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

This change seems to have broken two libc++ tests:

  libc++ :: std/utilities/any/any.nonmembers/any.cast/const_correctness.fail.cpp
  
  Script:
  --
  : 'COMPILED WITH';  /b/s/w/ir/x/w/staging/llvm_build/./bin/clang++ 
/b/s/w/ir/x/w/llvm-project/libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.fail.cpp
 -v --sysroot=/b/s/w/ir/x/w/cipd/linux --target=x86_64-unknown-linux-gnu 
-include /b/s/w/ir/x/w/llvm-project/libcxx/test/support/nasty_macros.h 
-nostdinc++ 
-I/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 
-I/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 
-I/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libcxx/include/c++build
 -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS 
-I/b/s/w/ir/x/w/llvm-project/libcxx/test/support -std=c++2b -Werror -Wall 
-Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes 
-Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals 
-Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable 
-Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef 
-D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety 
-Wuser-defined-warnings -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER 
-Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined 
-D_LIBCPP_ABI_VERSION=2  -fsyntax-only -Wno-error -Xclang -verify -Xclang 
-verify-ignore-unexpected=note -ferror-limit=0
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  $ ":" "COMPILED WITH"
  $ "/b/s/w/ir/x/w/staging/llvm_build/./bin/clang++" 
"/b/s/w/ir/x/w/llvm-project/libcxx/test/std/utilities/any/any.nonmembers/any.cast/const_correctness.fail.cpp"
 "-v" "--sysroot=/b/s/w/ir/x/w/cipd/linux" "--target=x86_64-unknown-linux-gnu" 
"-include" "/b/s/w/ir/x/w/llvm-project/libcxx/test/support/nasty_macros.h" 
"-nostdinc++" 
"-I/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1" 
"-I/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1" 
"-I/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libcxx/include/c++build"
 "-D__STDC_FORMAT_MACROS" "-D__STDC_LIMIT_MACROS" "-D__STDC_CONSTANT_MACROS" 
"-I/b/s/w/ir/x/w/llvm-project/libcxx/test/support" "-std=c++2b" "-Werror" 
"-Wall" "-Wextra" "-Wshadow" "-Wundef" "-Wno-unused-command-line-argument" 
"-Wno-attributes" "-Wno-pessimizing-move" "-Wno-c++11-extensions" 
"-Wno-user-defined-literals" "-Wno-noexcept-type" "-Wno-atomic-alignment" 
"-Wsign-compare" "-Wunused-variable" "-Wunused-parameter" "-Wunreachable-code" 
"-Wno-unused-local-typedef" "-D_LIBCPP_DISABLE_AVAILABILITY" "-fcoroutines-ts" 
"-Werror=thread-safety" "-Wuser-defined-warnings" 
"-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER" "-Wno-macro-redefined" 
"-D_LIBCPP_HAS_THREAD_API_PTHREAD" "-Wno-macro-redefined" 
"-D_LIBCPP_ABI_VERSION=2" "-fsyntax-only" "-Wno-error" "-Xclang" "-verify" 
"-Xclang" "-verify-ignore-unexpected=note" "-ferror-limit=0"
  # command stderr:
  Fuchsia clang version 14.0.0 (https://llvm.googlesource.com/a/llvm-project 
e1d6f29a1e640e267e1d2b94d0d761e1d15e99bd)
  Target: x86_64-unknown-linux-gnu
  Thread model: posix
  InstalledDir: /b/s/w/ir/x/w/staging/llvm_build/./bin
  Found candidate GCC installation: 
/b/s/w/ir/x/w/cipd/linux/usr/lib/gcc/i586-linux-gnu/4.8
  Found candidate GCC installation: 
/b/s/w/ir/x/w/cipd/linux/usr/lib/gcc/x86_64-linux-gnu/4.8
  Selected GCC installation: 
/b/s/w/ir/x/w/cipd/linux/usr/lib/gcc/x86_64-linux-gnu/4.8
  Candidate multilib: .;@m64
  Selected multilib: .;@m64
   (in-process)
   "/b/s/w/ir/x/w/staging/llvm_build/bin/clang-14" -cc1 -triple 
x86_64-unknown-linux-gnu -fsyntax-only -disable-free -clear-ast-before-backend 
-main-file-name const_correctness.fail.cpp -mrelocation-model static 
-mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math 
-mconstructor-aliases -funwind-tables=2 -target-cpu x86-64 -tune-cpu generic 
-debugger-tuning=gdb -v 
-fcoverage-compilation-dir=/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libcxx/test/std/utilities/any/any.nonmembers/any.cast
 -nostdinc++ -resource-dir /b/s/w/ir/x/w/staging/llvm_build/lib/clang/14.0.0 
-include /b/s/w/ir/x/w/llvm-project/libcxx/test/support/nasty_macros.h -I 
/b/s/w/ir/x/w/staging/llvm_build/include/x86_64-unknown-linux-gnu/c++/v1 -I 
/b/s/w/ir/x/w/staging/llvm_build/include/c++/v1 -I 
/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/libcxx/include/c++build
 -D __STDC_FORMAT_MACROS -D __STDC_LIMIT_MACROS -D __STDC_CONSTANT_MACROS -I 
/b/s/w/ir/x/w/llvm-project/libcxx/test/support -D _LIBCPP_DISABLE_AVAILABILITY 
-D _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D _LIBCPP_HAS_THREAD_API_PTHREAD -D 
_LIBCPP_ABI_VERSION=2 -isysroot /b/s/w/ir/x/w/cipd/linux -internal-isystem 
/b/s/w/ir/x/w/staging/llvm_build/lib/clang/14.0.0/include -internal-isystem 
/b/s/w/ir/x/w/cipd/linux/usr/local/include -internal-isystem 

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as done.
mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:559
+  // Get the resolved template arguments from the canonical type.
+  // FIXME: Handle the as-written arguments so the sugar is not lost.
+  ArrayRef PResolved =

rsmith wrote:
> Does this matter on the `P` side? (Can we remove this FIXME?)
Even though P sugar is not relevant to the deduced type, it is relevant to 
diagnostics when there is a deduction failure. This has been relevant in the 
other parts of this patch, so I don't see why we would not at least want to 
preserve it here, though it might be too much trouble to be worth the effort.

I will keep the comment for now, if we have any discussion where it becomes 
clear there would be no difference here, I will make a one line NFC and remove 
it.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:561
+  ArrayRef PResolved =
+  TP->getCanonicalTypeInternal()
+  ->castAs()

rsmith wrote:
> Can we avoid using the `Internal` version here? (The only difference is 
> whether we preserve top-level qualifiers that are outside any type sugar.)
> 
> Might also be worth a comment here explaining that we're going to the 
> canonical type first here to step over any alias templates.
The Internal version is more convenient because TP is a `Type *` not a 
`QualType`, so the suggestion as you have written would not work.

Removing the alias template would be handled by `getUnqualifiedDesugaredType`, 
above, which my understanding is that will remove all top level sugar.

But I see that I went all this way around to preserving the non-canonical TP 
but all I needed really from it was the templateName, which probably is always 
the same as the canonical one, though not sure if that is by design.

I just made a change to get the canonical type on TP above instead, and we can 
circle back to this later.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:571-573
+  // FIXME: Should not lose sugar here.
+  if (const auto *SA = dyn_cast(
+  UA->getCanonicalTypeInternal())) {

rsmith wrote:
> I think we can retain type sugar here without too much effort.
That does not work, blows up some 80 tests.
I will leave the FIXME and circle back to this after I am done with my pile of 
patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

Awesome!




Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:559
+  // Get the resolved template arguments from the canonical type.
+  // FIXME: Handle the as-written arguments so the sugar is not lost.
+  ArrayRef PResolved =

Does this matter on the `P` side? (Can we remove this FIXME?)



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:561
+  ArrayRef PResolved =
+  TP->getCanonicalTypeInternal()
+  ->castAs()

Can we avoid using the `Internal` version here? (The only difference is whether 
we preserve top-level qualifiers that are outside any type sugar.)

Might also be worth a comment here explaining that we're going to the canonical 
type first here to step over any alias templates.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:571-573
+  // FIXME: Should not lose sugar here.
+  if (const auto *SA = dyn_cast(
+  UA->getCanonicalTypeInternal())) {

I think we can retain type sugar here without too much effort.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:573
+  if (const auto *SA = dyn_cast(
+  UA->getCanonicalTypeInternal())) {
 // Perform template argument deduction for the template name.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3285
   }
-  if (const TypedefType *TypedefT =
-  dyn_cast(FromFPT->getReturnType())) {
-TypedefNameDecl *TD = TypedefT->getDecl();
+  if (const auto *TypedefT = dyn_cast(FromFPT->getReturnType())) {
+const TypedefNameDecl *TD = TypedefT->getDecl();

rsmith wrote:
> (Preparing for D112374)
Good catch, thanks :)



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1385-1388
+P = P.getUnqualifiedType();
 // - If A is a cv-qualified type, A is replaced by the cv-unqualified
 //   version of A.
+A = A.getUnqualifiedType();

rsmith wrote:
> Do we need to use `Context.getUnqualifiedArrayType` here, for cases like:
> ```
> typedef const int CInt;
> // Partial ordering should deduce `T = int` from both parameters here,
> // even though `T = const int` might make more sense for the first one.
> template void f(CInt (&)[N], int*);
> template void f(T (&)[N], T*);
> CInt arr[5];
> void g() { f(arr, arr); }
> ```
> ? I think `getUnqualifiedType()` on `CInt[N]` will not remove the indirect 
> `const` qualifier.
I see, that is true, if P and A could be sugared here, we would need 
`getUnqualifiedArrayType`.

But this issue had been sidestepped because we were not handling sugar in the 
PartialOrdering case, because that was only used in the isAtLeastAsSpecialized 
path where we were still using the canonical type, as we don't care about what 
types where actually deduced there.

I just changed it to not use the canonical type anymore and took this change.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1546-1568
+  QualType CanP = P.getCanonicalType();
+  // If the parameter type is not dependent, there is nothing to deduce.
+  if (!P->isDependentType()) {
+if (TDF & TDF_SkipNonDependent)
   return Sema::TDK_Success;
+
+QualType CanA = A.getCanonicalType();

rsmith wrote:
> I think we can excise `CanA` and `CanP` entirely here so people aren't 
> tempted to use them and lose sugar.
Nice suggestion!

Though we must keep the last return as conditional, as I did there, otherwise we
break a test case such as the one from PR12132:
```
template void fun(const int* const S::* member) {}
struct A { int* x; };
void foo() {
fun(::x);
}
```
When ignoring qualifiers, hasSameUnqualifiedType being false is not enough to 
deduce a mismatch, so this is the one corner case where we analyze with a 
non-dependent parameter type.

That is what I tried to explain in that comment, but I have just reworded it to 
be clearer on this point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.
Herald added a subscriber: carlosgalvezp.



Comment at: clang/include/clang/AST/Type.h:4968
   QualType getDeducedType() const {
-return !isCanonicalUnqualified() ? getCanonicalTypeInternal() : QualType();
+return DeducedAsType.isNull() ? QualType() : DeducedAsType;
   }





Comment at: clang/lib/AST/ASTImporter.cpp:3275
+!DeducedT.isNull()
+? dyn_cast(DeducedT->getCanonicalTypeInternal())
+: nullptr) {





Comment at: clang/lib/AST/ASTImporter.cpp:3285
   }
-  if (const TypedefType *TypedefT =
-  dyn_cast(FromFPT->getReturnType())) {
-TypedefNameDecl *TD = TypedefT->getDecl();
+  if (const auto *TypedefT = dyn_cast(FromFPT->getReturnType())) {
+const TypedefNameDecl *TD = TypedefT->getDecl();

(Preparing for D112374)



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1154
   if (!ParamFunction || !ArgFunction)
 return Param == Arg;
 

The `==` comparisons here and below should presumably be `hasSameType` 
comparisons now that we're preserving sugar.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1234-1250
   llvm::SmallPtrSet Visited;
   SmallVector ToVisit;
   // We iterate over this later, so we have to use MapVector to ensure
   // determinism.
   llvm::MapVector>
   Matches;
 

I think we should:

* track `ToVisit` as a vector of `QualType`, so we preserve the type sugar as 
written in base specifier lists
* track `Visited` as a set of (canonical) `const CXXRecordDecl*`s rather than 
relying on `RecordType`s always being canonical (they currently are, but I 
don't think that's intended to be guaranteed)



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1385-1388
+P = P.getUnqualifiedType();
 // - If A is a cv-qualified type, A is replaced by the cv-unqualified
 //   version of A.
+A = A.getUnqualifiedType();

Do we need to use `Context.getUnqualifiedArrayType` here, for cases like:
```
typedef const int CInt;
// Partial ordering should deduce `T = int` from both parameters here,
// even though `T = const int` might make more sense for the first one.
template void f(CInt (&)[N], int*);
template void f(T (&)[N], T*);
CInt arr[5];
void g() { f(arr, arr); }
```
? I think `getUnqualifiedType()` on `CInt[N]` will not remove the indirect 
`const` qualifier.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1437
 // top level, so they can be matched with the qualifiers on the parameter.
-if (isa(Arg)) {
+if (isa(A)) {
   Qualifiers Quals;

`A` could be sugar for an array type here.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1546-1568
+  QualType CanP = P.getCanonicalType();
+  // If the parameter type is not dependent, there is nothing to deduce.
+  if (!P->isDependentType()) {
+if (TDF & TDF_SkipNonDependent)
   return Sema::TDK_Success;
+
+QualType CanA = A.getCanonicalType();

I think we can excise `CanA` and `CanP` entirely here so people aren't tempted 
to use them and lose sugar.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1643
 
-  return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+  return ParDesug == ArgDesug ? Sema::TDK_Success
+  : Sema::TDK_NonDeducedMismatch;

mizvekov wrote:
> mizvekov wrote:
> > rsmith wrote:
> > > This looks wrong to me: we should be comparing the types, not how they're 
> > > written. `Context.hasSameType(Param, Arg)` (or 
> > > `Context.hasSameUnqualifiedType(Param, Arg)` in the 
> > > `TDF_IgnoreQualifiers` case) would be appropriate here.
> > You are right, but the reason we don't get into any troubles here is 
> > because this is dead code anyway, the non-dependent case will always be 
> > handled above :)
> > 
> > Although perhaps, I wonder if we should dig down into non-dependent types 
> > anyway, in case the types are too complex and it's not immediately obvious 
> > what does not match, we could perhaps improve the diagnostic?
> > 
> > I will experiment a little bit with this idea.
> Here are the results of this experiment:
> ```
> error: 'note' diagnostics expected but not seen:
>   File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: could not match 
> 'void (I) noexcept(false)' (aka 'void (int) noexcept(false)') against 'void 
> (int) noexcept'
> error: 'note' diagnostics seen but not expected:
>   File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: candidate template 
> ignored: failed template argument deduction
> 
> error: 'note' diagnostics expected but not seen:
>   File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template 
> ignored: could not match 'auto ()' against 'int ()'
>   File 

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:250
 Context.hasSameType(X.getAsType(), Y.getAsType()))
   return X;
 

mizvekov wrote:
> rsmith wrote:
> > I wonder if we can come up with a good heuristic for determining which of 
> > `X` and `Y` we want to return here. One thing that immediately springs to 
> > mind is: if either type is canonical, we should return the other one.
> I was thinking about the opposite actually, something along the lines of 
> returning the common sugar between them. Starting from the cannonical type 
> and going 'backwards' (adding back the sugar), we return the type just before 
> adding sugar back would make them different.
I have a proposal for what to do here in this patch: 
https://reviews.llvm.org/D111283

I think it's ready except that it needs more manual labor to support digging 
down into more types.

Anyway, this patch above refines the behavior of the present one, by 
implementing a criteria to fold the type sugar when deduction happens from 
multiple arguments,
or in the case of deducing from auto returning function, to fold them from the 
multiple return statements.

Otherwise, as in the present patch, the sugar is considered only from the first 
argument deduced from.

So any stakeholders, please subscribe and provide feedback about the rules as 
they are implemented :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3044102 , @aaronpuchert 
wrote:

> Unless I'm missing something, alignment on aliases is itself fragile. Let's 
> say you have some template, then the instantiation with the alias is of 
> course the same as the instantiation with the canonical type, so if that 
> template has a member or local variable of that type parameter that won't get 
> the alignment.

If the attribute was not just type sugar and could be made part of the 
canonical type, then the attribute would have to be attached to the canonical 
type of the alias when building it,
in a similar way to how you don't lose other structural parts of the type when 
canonicalizing it.

But then you would have to deal with the whole mess this would cause in the 
rest of clang :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D110216#3041117 , @mizvekov wrote:

> In D110216#3040748 , @craig.topper 
> wrote:
>
>> Looks like this fixes PR51282.
>
> I guess it does fix it, but the underlying implementation of alignment is 
> very fragile. The alignment is stored in `AttributedType` nodes, which are 
> always sugar, so they are not considered part of the type in a structural 
> sense...

Unless I'm missing something, alignment on aliases is itself fragile. Let's say 
you have some template, then the instantiation with the alias is of course the 
same as the instantiation with the canonical type, so if that template has a 
member or local variable of that type parameter that won't get the alignment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3040748 , @craig.topper 
wrote:

> Looks like this fixes PR51282.

I guess it does fix it, but the underlying implementation of alignment is very 
fragile. The alignment is stored in `AttributedType` nodes, which are always 
sugar, so they are not considered part of the type in a structural sense...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as done.
mizvekov added a comment.

In D110216#3040748 , @craig.topper 
wrote:

> Looks like this fixes PR51282.

Okay, interesting. This could be just papering over the bug.
Do you still lose this alignment if it goes through a template argument?
If you do, this looks like a canonicalization bug.




Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1032
 if (const auto *S = dyn_cast()) {
-  EffectiveType = S->getDeducedType().getTypePtrOrNull();
-  if (!EffectiveType)
-return false;
+  QualType T = S->getDeducedType();
+  return !T.isNull() ? matchesSpecialized(*T, Finder, Builder) : false;

craig.topper wrote:
> gcc 8.4.1 doesn't like `T` being the same name as a template parameter for 
> this class.
Oh wow, didn't even realize, thanks!!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Looks like this fixes PR51282.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchersInternal.h:1032
 if (const auto *S = dyn_cast()) {
-  EffectiveType = S->getDeducedType().getTypePtrOrNull();
-  if (!EffectiveType)
-return false;
+  QualType T = S->getDeducedType();
+  return !T.isNull() ? matchesSpecialized(*T, Finder, Builder) : false;

gcc 8.4.1 doesn't like `T` being the same name as a template parameter for this 
class.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

In D110216#3038826 , @mizvekov wrote:

> In D110216#3038797 , @v.g.vassilev 
> wrote:
>
>> Over the years we had some interest from people but never actually got 
>> implemented. Here 
>>  were some 
>> ideas @rsmith and I discussed over the years. If that is helpful, let me 
>> know if I should dig a bit more into private email exchange.
>
> Sure, that is helpful :)
>
> There is other lower hanging fruit where we are losing sugar, and it would be 
> a shame if we implemented this but then did not get much benefit from it 
> because the sugar never got into the template argument in the first place.
>
> One such example is that we do not mark as 'elaborate' types which are 
> written bare, without any scope specifiers.
>
> So for example in a case like this:
>
>   namespace foo {
> struct Foo {};
> Foo x = 0;
>   };
>
> We would still diagnose that assignment with the type of the variable printed 
> as 'foo::Foo' instead of just 'Foo', as it was written, because the parser 
> will have produced a type that is not wrapped in an ElaboratedType (or 
> perhaps some other cheaper mechanism).

Handling that case is nice. I am more interested in retaining the sugar in 
template instantiations as it is essential for an optimization in our I/O 
system which uses clang as a library. If implement the instantiation diagnostic 
we can get rid of several hacky patches: 
https://github.com/vgvassilev/clang/commit/d87e2bbc8a266e295ee5a2065f1e587b325d4284
 
https://github.com/vgvassilev/clang/commit/fcc492fcab14e8b8dc156688dda6f237a04563a7
 and 
https://github.com/vgvassilev/clang/commit/fe17b953325530267643f3391bfd59ac1519ef39




Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1414
   TDF &= ~TDF_TopLevelParameterTypeList;
-  if (isForwardingReference(Param, 0) && Arg->isLValueReferenceType())
-Param = Param->getPointeeType();
+  if (isForwardingReference(P, 0) && A->isLValueReferenceType())
+P = P->getPointeeType();





Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1777
   S, TemplateParams, NTTP, Noexcept, S.Context.BoolTy,
-  /*ArrayBound*/true, Info, Deduced);
+  /*ArrayBound*/ true, Info, Deduced);
 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3038797 , @v.g.vassilev 
wrote:

> Over the years we had some interest from people but never actually got 
> implemented. Here 
>  were some 
> ideas @rsmith and I discussed over the years. If that is helpful, let me know 
> if I should dig a bit more into private email exchange.

Sure, that is helpful :)

There is other lower hanging fruit where we are losing sugar, and it would be a 
shame if we implemented this but then did not get much benefit from it because 
the sugar never got into the template argument in the first place.

One such example is that we do not mark as 'elaborate' types which are written 
bare, without any scope specifiers.

So for example in a case like this:

  namespace foo {
struct Foo {};
Foo x = 0;
  };

We would still diagnose that assignment with the type of the variable printed 
as 'foo::Foo' instead of just 'Foo', as it was written, because the parser will 
have produced a type that is not wrapped in an ElaboratedType (or perhaps some 
other cheaper mechanism).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

In D110216#3035041 , @mizvekov wrote:

> In D110216#3032626 , @v.g.vassilev 
> wrote:
>
>> Thanks for working on this!How hard would it be to support:
>>
>>   using size_t = __SIZE_TYPE__;
>>   template struct Id { typedef T type; };
>>   int main() {
>> struct S {} s;
>> Id::type f = s; // just 'unsigned long', 'size_t' sugar has been 
>> lost
>>   }
>
> Actually supporting that is in my radar :)

Over the years we had some interest from people but never actually got 
implemented. Here 
 were some 
ideas @rsmith and I discussed over the years. If that is helpful, let me know 
if I should dig a bit more into private email exchange.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked 12 inline comments as done.
mizvekov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:180
   )cpp",
-  // FIXME: it'd be nice if this resolved to the alias instead
-  "struct Foo",
+  // It's so nice that this is resolved to the alias instead :-D
+  "Bar",

rsmith wrote:
> Very true. But probably not worth keeping the comment. :)
Such a party pooper =)



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1350-1362
+  QualType ParDesug;
+  auto updatePar = [, , ](QualType T) {
+Param = T;
+ParDesug = T.getDesugaredType(S.Context);
+  };
+  updatePar(Param);
+

rsmith wrote:
> rsmith wrote:
> > `Par` is an unusual name for a parameter type; `Parm` or `Param` would be 
> > more common and I'd find either of those to be clearer. (Ideally use the 
> > same spelling for `Param` and `ParDesug`.) Given that the description in 
> > the standard uses `P` and `A` as the names of these types, those names 
> > would be fine here too if you want something short.
> Instead of tracking two types here, I think it'd be clearer to follow the 
> more usual pattern in Clang: track only `Param` and `Arg`, use 
> `Arg->getAs()` instead of `dyn_cast(ArgDesug)` to identify if `Arg` is 
> sugar for a `T`, and use `Arg->getAs()` instead of `cast(ArgDesug)` to 
> get a minimally-desugared `T*` from `Arg`.
> 
> The only place where I think we'd want something different from that is in 
> the `switch (Param->getTypeClass())` below, where I think we should switch on 
> the type class of the canonical type (or equivalently on the type class of 
> the desugared type, but it's cheaper and more obviously correct to ask for 
> the type class of the canonical type).
There was one small catch in your suggestion:
Using getAs on TemplateSpecializationType is a bit tricky because that type can 
be sugar as well, so you might end up with a type alias instead of the 
underlying thing you wanted. I think that was the only tricky type though.
And it was productive that I ran into this problem, because I ended up 
discovering some cases where we were losing some sugar there, and there was a 
tiny diagnostic improvement in one of the test cases as a result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1643
 
-  return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+  return ParDesug == ArgDesug ? Sema::TDK_Success
+  : Sema::TDK_NonDeducedMismatch;

mizvekov wrote:
> rsmith wrote:
> > This looks wrong to me: we should be comparing the types, not how they're 
> > written. `Context.hasSameType(Param, Arg)` (or 
> > `Context.hasSameUnqualifiedType(Param, Arg)` in the `TDF_IgnoreQualifiers` 
> > case) would be appropriate here.
> You are right, but the reason we don't get into any troubles here is because 
> this is dead code anyway, the non-dependent case will always be handled above 
> :)
> 
> Although perhaps, I wonder if we should dig down into non-dependent types 
> anyway, in case the types are too complex and it's not immediately obvious 
> what does not match, we could perhaps improve the diagnostic?
> 
> I will experiment a little bit with this idea.
Here are the results of this experiment:
```
error: 'note' diagnostics expected but not seen:
  File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: could not match 'void 
(I) noexcept(false)' (aka 'void (int) noexcept(false)') against 'void (int) 
noexcept'
error: 'note' diagnostics seen but not expected:
  File SemaCXX\cxx1z-noexcept-function-type.cpp Line 21: candidate template 
ignored: failed template argument deduction

error: 'note' diagnostics expected but not seen:
  File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template 
ignored: could not match 'auto ()' against 'int ()'
  File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template 
ignored: could not match 'auto ()' against 'void ()'
error: 'note' diagnostics seen but not expected:
  File SemaCXX\deduced-return-type-cxx14.cpp Line 146: candidate template 
ignored: could not match 'auto' against 'int'
  File SemaCXX\deduced-return-type-cxx14.cpp Line 161: candidate template 
ignored: could not match 'auto' against 'void'

error: 'note' diagnostics expected but not seen:
  File SemaCXX\pass-object-size.cpp Line 62 (directive at 
SemaCXX\pass-object-size.cpp:69): candidate address cannot be taken because 
parameter 1 has pass_object_size attribute
  File SemaCXX\pass-object-size.cpp Line 56 (directive at 
SemaCXX\pass-object-size.cpp:74): candidate address cannot be taken because 
parameter 1 has pass_object_size attribute
  File SemaCXX\pass-object-size.cpp Line 62 (directive at 
SemaCXX\pass-object-size.cpp:75): candidate address cannot be taken because 
parameter 1 has pass_object_size attribute
error: 'note' diagnostics seen but not expected:
  File SemaCXX\pass-object-size.cpp Line 56: candidate template ignored: failed 
template argument deduction
  File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored: failed 
template argument deduction
  File SemaCXX\pass-object-size.cpp Line 62: candidate template ignored: failed 
template argument deduction

error: 'note' diagnostics expected but not seen:
  File SemaTemplate\deduction.cpp Line 316: deduced non-type template argument 
does not have the same type as the corresponding template parameter 
('std::nullptr_t' vs 'int *')
  File SemaTemplate\deduction.cpp Line 323: values of conflicting types
error: 'note' diagnostics seen but not expected:
  File SemaTemplate\deduction.cpp Line 275: candidate template ignored: could 
not match 'const int' against 'int'
  File SemaTemplate\deduction.cpp Line 316: candidate template ignored: could 
not match 'int *' against 'std::nullptr_t'
  File SemaTemplate\deduction.cpp Line 323: candidate template ignored: could 
not match 'int *' against 'std::nullptr_t'

error: 'note' diagnostics expected but not seen:
  File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template 
ignored: could not match 'void ()' against 'void (float *)'
  File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template 
ignored: could not match 'void (int *)' against 'void (float *)'
error: 'note' diagnostics seen but not expected:
  File SemaTemplate\explicit-instantiation.cpp Line 70: candidate template 
ignored: could not match 'int' against 'float'
  File SemaTemplate\explicit-instantiation.cpp Line 64: candidate template 
ignored: failed template argument deduction
```

It's interesting to note that it reveals several cases we give too generic 
'failed template argument deduction' errors, like the different noexcept 
specifications, function prototypes with different amount of parameters, and 
the 'pass_object_size attribute' case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1643
 
-  return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+  return ParDesug == ArgDesug ? Sema::TDK_Success
+  : Sema::TDK_NonDeducedMismatch;

rsmith wrote:
> This looks wrong to me: we should be comparing the types, not how they're 
> written. `Context.hasSameType(Param, Arg)` (or 
> `Context.hasSameUnqualifiedType(Param, Arg)` in the `TDF_IgnoreQualifiers` 
> case) would be appropriate here.
You are right, but the reason we don't get into any troubles here is because 
this is dead code anyway, the non-dependent case will always be handled above :)

Although perhaps, I wonder if we should dig down into non-dependent types 
anyway, in case the types are too complex and it's not immediately obvious what 
does not match, we could perhaps improve the diagnostic?

I will experiment a little bit with this idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3032626 , @v.g.vassilev 
wrote:

> Thanks for working on this!How hard would it be to support:
>
>   using size_t = __SIZE_TYPE__;
>   template struct Id { typedef T type; };
>   int main() {
> struct S {} s;
> Id::type f = s; // just 'unsigned long', 'size_t' sugar has been 
> lost
>   }

Actually supporting that is in my radar :)

Getting the basic idea up is not too complicated.
We need to support 'alias' template specializations which can carry 
non-canonical arguments,
and they would just point to the canonical template specializations.
We would then have to implement our desugaring of 'SubstTemplateTypeParmType' 
so it's able to access some context where it can get the correctly sugared 
ReplacementType for the given replaced 'TemplateTypeParmType'.

But there are some little details that need to be implemented so that it is 
reasonably usable.
For example, in the general case we cannot currently, for any given 
declaration, figure out the template arguments used to instantiate it. We can 
do it for class and function templates, but not for alias and variable 
templates.
We need to make anything that carries template arguments also be a declaration 
context, so we can walk up from any declaration and see those.

There is also the trouble that some elements are not properly parented. For 
example in a requires clause:
`template  requires WHATEVER struct X {}`, WHATEVER is not currently 
parented to the class template, so we would not be able to recover the 
instantiation arguments there unless we fixed it.

And who knows what other non-obvious problems might be lurking :)

A lot of what I said also overlaps with what we need to properly support lazy 
substitution, such as required by C++20 concepts, which is also something I am 
working on.

Getting the 'template argument deduction' machinery to preserve the sugar, 
which we do in this patch, certainly benefits when we get there.

In D110216#3031531 , @rsmith wrote:

> Thanks, this is very exciting, and the diagnostic changes in the test suite 
> look great!

Thank you, much appreciated, and also thanks for the review!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp:276
+
+auto NotOkay5 = []() { U u; return u.getBadLambda(); }();
+// CHECK-EXCEPTIONS: :[[@LINE-1]]:6: warning: initialization of 'NotOkay5' 
with static storage duration may throw an exception that cannot be caught 
[cert-err58-cpp]

rsmith wrote:
> What happened here? It looks like nothing this expression does can actually 
> throw. (This lambda's `operator()` isn't `noexcept`, but the functions it 
> calls all are.) Was this only avoiding a false positive by accident before?
I haven't debugged it, I was hoping @aaron.ballman  would know something about 
it and it would save me the trouble :)



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:250
 Context.hasSameType(X.getAsType(), Y.getAsType()))
   return X;
 

rsmith wrote:
> I wonder if we can come up with a good heuristic for determining which of `X` 
> and `Y` we want to return here. One thing that immediately springs to mind 
> is: if either type is canonical, we should return the other one.
I was thinking about the opposite actually, something along the lines of 
returning the common sugar between them. Starting from the cannonical type and 
going 'backwards' (adding back the sugar), we return the type just before 
adding sugar back would make them different.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1051-1052
+  DeduceTemplateArgumentsByTypeMatch(
+  S, TemplateParams, Params[ParamIdx].getUnqualifiedType(),
+  Args[ArgIdx].getUnqualifiedType(), Info, Deduced, TDF,
+  PartialOrdering,

rsmith wrote:
> Hm, do we need the `getUnqualifiedType()` calls here? I'd expect that we'd 
> have the `TDF_IgnoreQualifiers` flag set in this case, so we should be 
> ignoring qualifiers anyway. Perhaps `DeduceTemplateArgumentsByTypeMatch` 
> should be removing qualifiers itself if that flag is set?
The case I am worried about is checking the parameters in the function 
prototype case, and `TDF_IgnoreQualifiers` is neither set there, nor it would 
help if it was set anyway.
I will take a look again, but this was the simplest solution I could come up at 
the time.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1811-1812
+  S, TemplateParams,
+  FunctionProtoParam->getReturnType().getUnqualifiedType(),
+  FunctionProtoArg->getReturnType().getUnqualifiedType(), Info,
+  Deduced, 0,

rsmith wrote:
> Ignoring qualifiers here doesn't seem right to me. For example:
> ```
> template void f(T(T));
> const int 

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

Thanks for working on this! How hard would it be to support:

  using size_t = __SIZE_TYPE__;
  template struct Id { typedef T type; };
  int main() {
struct S {} s;
Id::type f = s; // just 'unsigned long', 'size_t' sugar has been 
lost
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this is very exciting, and the diagnostic changes in the test suite 
look great!




Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:180
   )cpp",
-  // FIXME: it'd be nice if this resolved to the alias instead
-  "struct Foo",
+  // It's so nice that this is resolved to the alias instead :-D
+  "Bar",

Very true. But probably not worth keeping the comment. :)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp:276
+
+auto NotOkay5 = []() { U u; return u.getBadLambda(); }();
+// CHECK-EXCEPTIONS: :[[@LINE-1]]:6: warning: initialization of 'NotOkay5' 
with static storage duration may throw an exception that cannot be caught 
[cert-err58-cpp]

What happened here? It looks like nothing this expression does can actually 
throw. (This lambda's `operator()` isn't `noexcept`, but the functions it calls 
all are.) Was this only avoiding a false positive by accident before?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp:94-96
+  auto owned_int7 = returns_owner1(); // Ok, since type deduction does not 
eliminate the owner wrapper
 
+  const auto owned_int8 = returns_owner2(); // Ok, since type deduction does 
not eliminate the owner wrapper

@JonasToth What was the intent here -- is there a specification for how 
`gsl::owner` is supposed to interact with `auto` deduction? That is, is this a 
bug fix or a regression?



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:250
 Context.hasSameType(X.getAsType(), Y.getAsType()))
   return X;
 

I wonder if we can come up with a good heuristic for determining which of `X` 
and `Y` we want to return here. One thing that immediately springs to mind is: 
if either type is canonical, we should return the other one.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1051-1052
+  DeduceTemplateArgumentsByTypeMatch(
+  S, TemplateParams, Params[ParamIdx].getUnqualifiedType(),
+  Args[ArgIdx].getUnqualifiedType(), Info, Deduced, TDF,
+  PartialOrdering,

Hm, do we need the `getUnqualifiedType()` calls here? I'd expect that we'd have 
the `TDF_IgnoreQualifiers` flag set in this case, so we should be ignoring 
qualifiers anyway. Perhaps `DeduceTemplateArgumentsByTypeMatch` should be 
removing qualifiers itself if that flag is set?



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1350
+bool PartialOrdering, bool DeducedFromArrayBound) {
+  QualType ParDesug;
+  auto updatePar = [, , ](QualType T) {

`Par` is an unusual name for a parameter type; `Parm` or `Param` would be more 
common and I'd find either of those to be clearer. (Ideally use the same 
spelling for `Param` and `ParDesug`.) Given that the description in the 
standard uses `P` and `A` as the names of these types, those names would be 
fine here too if you want something short.



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1350-1362
+  QualType ParDesug;
+  auto updatePar = [, , ](QualType T) {
+Param = T;
+ParDesug = T.getDesugaredType(S.Context);
+  };
+  updatePar(Param);
+

rsmith wrote:
> `Par` is an unusual name for a parameter type; `Parm` or `Param` would be 
> more common and I'd find either of those to be clearer. (Ideally use the same 
> spelling for `Param` and `ParDesug`.) Given that the description in the 
> standard uses `P` and `A` as the names of these types, those names would be 
> fine here too if you want something short.
Instead of tracking two types here, I think it'd be clearer to follow the more 
usual pattern in Clang: track only `Param` and `Arg`, use `Arg->getAs()` 
instead of `dyn_cast(ArgDesug)` to identify if `Arg` is sugar for a `T`, and 
use `Arg->getAs()` instead of `cast(ArgDesug)` to get a 
minimally-desugared `T*` from `Arg`.

The only place where I think we'd want something different from that is in the 
`switch (Param->getTypeClass())` below, where I think we should switch on the 
type class of the canonical type (or equivalently on the type class of the 
desugared type, but it's cheaper and more obviously correct to ask for the type 
class of the canonical type).



Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1561-1562
   // Check the cv-qualifiers on the parameter and argument types.
   CanQualType CanParam = S.Context.getCanonicalType(Param);
   CanQualType CanArg = S.Context.getCanonicalType(Arg);
   if (!(TDF & TDF_IgnoreQualifiers)) {

Can we remove these? It looks like we're almost not using them at all, and that 
we shouldn't need to use them: any type matching should work equally well with 
the desugared type, and we never want to include 

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+  ExpectedHint{": S1", "x"},
+  ExpectedHint{": S2::Inner", "y"});
 }

mizvekov wrote:
> nridge wrote:
> > mizvekov wrote:
> > > mizvekov wrote:
> > > > nridge wrote:
> > > > > This test expectation change is suspicious. The type is being printed 
> > > > > with `PrintingPolicy::SuppressScope=true`, shouldn't that still be 
> > > > > respected?
> > > > Thanks for pointing that out, I will take a look, but I suspect this to 
> > > > be some TypePrinter issue.
> > > Could you explain to me why the former behavior is more desirable here?
> > > 
> > > I initially understood that this hint should provide the type written in 
> > > a form that would actually work if it replaced the 'auto'.
> > > It is strange, but if it is just meant as a hint, it's still fine I guess.
> > > 
> > > Or maybe this was broken in the first place and just was just missing a 
> > > FIXME?
> > The type is being printed with a `PrintingPolicy` which has the 
> > [non-default SuppressScope flag 
> > set](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang-tools-extra/clangd/InlayHints.cpp#34).
> > 
> > The 
> > [documentation](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang/include/clang/AST/PrettyPrinter.h#129)
> >  of this flag says "Suppresses printing of scope specifiers", which I 
> > understand to mean both namespace and class scope specifiers.
> > 
> > If you're asking why we are using this flag for inlay hints, it's to keep 
> > the hints short so they don't take up too much horizontal space. Since the 
> > hints are displayed inline, hints that are too long can result in the line 
> > of code visually extending past the width of the editor window, and we try 
> > to minimize the impact of this.
> I see.
> 
> The problem is that the ElaboratedType sugar does not respect this when 
> printing the nested name specifier it contains.
> The quick fix of trying to make it respect it shows that there are a bunch of 
> tests that expect it not to.
> It seems that specific policy is used in places that need it to recover the 
> type as written.
> 
> So if we want this behavior, we probably need to extend the printing policy 
> with a new flag here.
Thanks for the explanation!

I think this test expectation change is fine then, and would just suggest 
adding a "// FIXME: SuppressScope is not respected in this case" comment 
perhaps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+  ExpectedHint{": S1", "x"},
+  ExpectedHint{": S2::Inner", "y"});
 }

nridge wrote:
> mizvekov wrote:
> > mizvekov wrote:
> > > nridge wrote:
> > > > This test expectation change is suspicious. The type is being printed 
> > > > with `PrintingPolicy::SuppressScope=true`, shouldn't that still be 
> > > > respected?
> > > Thanks for pointing that out, I will take a look, but I suspect this to 
> > > be some TypePrinter issue.
> > Could you explain to me why the former behavior is more desirable here?
> > 
> > I initially understood that this hint should provide the type written in a 
> > form that would actually work if it replaced the 'auto'.
> > It is strange, but if it is just meant as a hint, it's still fine I guess.
> > 
> > Or maybe this was broken in the first place and just was just missing a 
> > FIXME?
> The type is being printed with a `PrintingPolicy` which has the [non-default 
> SuppressScope flag 
> set](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang-tools-extra/clangd/InlayHints.cpp#34).
> 
> The 
> [documentation](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang/include/clang/AST/PrettyPrinter.h#129)
>  of this flag says "Suppresses printing of scope specifiers", which I 
> understand to mean both namespace and class scope specifiers.
> 
> If you're asking why we are using this flag for inlay hints, it's to keep the 
> hints short so they don't take up too much horizontal space. Since the hints 
> are displayed inline, hints that are too long can result in the line of code 
> visually extending past the width of the editor window, and we try to 
> minimize the impact of this.
I see.

The problem is that the ElaboratedType sugar does not respect this when 
printing the nested name specifier it contains.
The quick fix of trying to make it respect it shows that there are a bunch of 
tests that expect it not to.
It seems that specific policy is used in places that need it to recover the 
type as written.

So if we want this behavior, we probably need to extend the printing policy 
with a new flag here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+  ExpectedHint{": S1", "x"},
+  ExpectedHint{": S2::Inner", "y"});
 }

mizvekov wrote:
> mizvekov wrote:
> > nridge wrote:
> > > This test expectation change is suspicious. The type is being printed 
> > > with `PrintingPolicy::SuppressScope=true`, shouldn't that still be 
> > > respected?
> > Thanks for pointing that out, I will take a look, but I suspect this to be 
> > some TypePrinter issue.
> Could you explain to me why the former behavior is more desirable here?
> 
> I initially understood that this hint should provide the type written in a 
> form that would actually work if it replaced the 'auto'.
> It is strange, but if it is just meant as a hint, it's still fine I guess.
> 
> Or maybe this was broken in the first place and just was just missing a FIXME?
The type is being printed with a `PrintingPolicy` which has the [non-default 
SuppressScope flag 
set](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang-tools-extra/clangd/InlayHints.cpp#34).

The 
[documentation](https://searchfox.org/llvm/rev/e158b5634aa67ea3039a62c3d8bda79b77b3b21c/clang/include/clang/AST/PrettyPrinter.h#129)
 of this flag says "Suppresses printing of scope specifiers", which I 
understand to mean both namespace and class scope specifiers.

If you're asking why we are using this flag for inlay hints, it's to keep the 
hints short so they don't take up too much horizontal space. Since the hints 
are displayed inline, hints that are too long can result in the line of code 
visually extending past the width of the editor window, and we try to minimize 
the impact of this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as not done.
mizvekov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+  ExpectedHint{": S1", "x"},
+  ExpectedHint{": S2::Inner", "y"});
 }

mizvekov wrote:
> nridge wrote:
> > This test expectation change is suspicious. The type is being printed with 
> > `PrintingPolicy::SuppressScope=true`, shouldn't that still be respected?
> Thanks for pointing that out, I will take a look, but I suspect this to be 
> some TypePrinter issue.
Could you explain to me why the former behavior is more desirable here?

I initially understood that this hint should provide the type written in a form 
that would actually work if it replaced the 'auto'.
It is strange, but if it is just meant as a hint, it's still fine I guess.

Or maybe this was broken in the first place and just was just missing a FIXME?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

FYI I just realized some other tests I never run locally relied on the former 
behavior of the ElaboratedType printer.

I will see what looks more reasonable, but I think the defaults are tuned so 
the type printer will output code that looks close to what it was as written,
and this InlayHints was relying on behavior that was not actually supported 
there. We might have to split this off into a separate policy flag or something 
like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked an inline comment as done.
mizvekov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+  ExpectedHint{": S1", "x"},
+  ExpectedHint{": S2::Inner", "y"});
 }

nridge wrote:
> This test expectation change is suspicious. The type is being printed with 
> `PrintingPolicy::SuppressScope=true`, shouldn't that still be respected?
Thanks for pointing that out, I will take a look, but I suspect this to be some 
TypePrinter issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/unittests/InlayHintTests.cpp:470
+  ExpectedHint{": S1", "x"},
+  ExpectedHint{": S2::Inner", "y"});
 }

This test expectation change is suspicious. The type is being printed with 
`PrintingPolicy::SuppressScope=true`, shouldn't that still be respected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 374921.
mizvekov added a comment.

update print-type.cpp test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
  clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaCXXScopeSpec.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaType.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/over/over.match/over.match.funcs/over.match.class.deduct/p2.cpp
  clang/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/multi-level-substitution.cpp
  clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p1-0x.cpp
  clang/test/Index/print-type.cpp
  clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
  clang/test/SemaCXX/cxx1z-decomposition.cpp
  clang/test/SemaCXX/deduced-return-type-cxx14.cpp
  clang/test/SemaCXX/friend.cpp
  clang/test/SemaCXX/recovery-expr-type.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/sugared-auto.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/test/SemaTemplate/friend.cpp
  clang/test/SemaTemplate/operator-template.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6176,26 +6176,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
-}
-
-TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredInside2) {
-  Decl *FromTU = getTuDecl(
-  R"(
-  auto foo() {
-struct X {};
-return X();
-  }
-  )",
-  Lang_CXX14, "input0.cc");
-  FunctionDecl *From =
-  FirstDeclMatcher().match(FromTU, functionDecl());
-
-  // This time import the type directly.
-  QualType ToT = ImportType(From->getType(), From, Lang_CXX14);
-  const FunctionProtoType *FPT = cast(ToT);
-  EXPECT_TRUE(isa(FPT->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefToStructDeclaredInside) {
@@ -6212,8 +6195,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredNestedInside) {
@@ -6229,8 +6213,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithInternalLambdaType) {
@@ -6249,8 +6234,9 @@
   FromTU, functionDecl(hasName("f")));
 
   FunctionDecl *To = Import(From, Lang_CXX17);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithTypeInIf) {
@@ -6268,8 +6254,9 @@
   FromTU, functionDecl(hasName("f")));
 
   FunctionDecl *To = Import(From, Lang_CXX17);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithTypeInFor) {
@@ -6285,8 +6272,9 @@
   FromTU, functionDecl(hasName("f")));
 
   FunctionDecl *To = 

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:495
+   const QualType ,
+   bool Underlying = false) {
   const auto  = AST.getSourceManager();

sammccall wrote:
> (i'm confused by the optional parameter - there's only one caller)
Oh I must have just assumed there were other callers, will fix.



Comment at: clang-tools-extra/clangd/XRefs.cpp:506
+  targetDecl(DynTypedNode::create(Type.getNonReferenceType()),
+ DeclRelation::TemplatePattern | DeclRelation::Alias |
+ (Underlying ? DeclRelation::Underlying : DeclRelation()),

sammccall wrote:
> `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.
I think that one is to make sure we can look through extra DeducedType sugar 
when finding some definition.
Specifically a case like the 'auto on lamda' from XRefsTests:
```
auto x = [[[]]]{};
^auto y = x;
```
`y` will have an extra layer of AutoType sugar there, since it deduced from 
something which already had AutoType sugar.

But maybe the way I implemented that in clangd is not good. I will take a look 
again, thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D110216#3019070 , @sammccall wrote:

> 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 
> :-(

Thanks!

> 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.

I think this will take a while to get merged, both because it's a big change in 
an obscure area, and also because there are some regressions in the ASTImporter 
support which I am not sure what their impact are, and if so the underlying 
problems there would need to be fixed first.

> 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).

I am a bit thorn on that issue as well. Ideally we may want to keep that kind 
of information because it might be useful for some kind analysis. On the other 
hand it is mostly a distraction for HMI purposes.

You raise a valid point that it is a bit silly to have this for decltype(auto) 
but not C++14 auto and C++11 auto, but I think the reason we have it in the 
first place was just an engineering / economical purpose: We had the code that 
built it from an expression as a monolith, and it was easier to just use it, 
than to expose the machinery that just calculates the underlying type. And when 
storing it on the AutoType, that sugar would be lost anyway.

Maybe It would be useful to create such nodes for the other autos as well, but 
then we would need a spelling for it, unlike decltype which already is in the 
vocabulary :)

> 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?

I think the only case we could possibly want to not skip when presenting to the 
user is not only when it is just dependent, but specifically when it is 
DependentTy (aka some completely unknown type (aka `T::foo`)).

As things stand, whenever we se an AutoType which is a `decltype(auto)`, and 
which has a non-null DeducedAsType, a decltype node is guaranteed to follow.

Perhaps we can store the expression in the AutoType itself instead? Though that 
is a big change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Sam McCall via Phabricator via cfe-commits]

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 ,
+   bool Underlying = false) {
   const auto  = 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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov updated this revision to Diff 374625.
mizvekov added a comment.

adjust column number on cert-static-object-exception test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110216

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
  clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p7-cxx14.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4-cxx14.cpp
  clang/test/CXX/over/over.match/over.match.funcs/over.match.class.deduct/p2.cpp
  clang/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/multi-level-substitution.cpp
  clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p1-0x.cpp
  clang/test/Index/print-type.cpp
  clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
  clang/test/SemaCXX/cxx1z-decomposition.cpp
  clang/test/SemaCXX/deduced-return-type-cxx14.cpp
  clang/test/SemaCXX/friend.cpp
  clang/test/SemaCXX/recovery-expr-type.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/sugared-auto.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/test/SemaTemplate/friend.cpp
  clang/test/SemaTemplate/operator-template.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6176,26 +6176,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
-}
-
-TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredInside2) {
-  Decl *FromTU = getTuDecl(
-  R"(
-  auto foo() {
-struct X {};
-return X();
-  }
-  )",
-  Lang_CXX14, "input0.cc");
-  FunctionDecl *From =
-  FirstDeclMatcher().match(FromTU, functionDecl());
-
-  // This time import the type directly.
-  QualType ToT = ImportType(From->getType(), From, Lang_CXX14);
-  const FunctionProtoType *FPT = cast(ToT);
-  EXPECT_TRUE(isa(FPT->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefToStructDeclaredInside) {
@@ -6212,8 +6195,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredNestedInside) {
@@ -6229,8 +6213,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithInternalLambdaType) {
@@ -6249,8 +6234,9 @@
   FromTU, functionDecl(hasName("f")));
 
   FunctionDecl *To = Import(From, Lang_CXX17);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithTypeInIf) {
@@ -6268,8 +6254,9 @@
   FromTU, functionDecl(hasName("f")));
 
   FunctionDecl *To = Import(From, Lang_CXX17);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithTypeInFor) {
@@ -6285,8 +6272,9 @@
   FromTU, functionDecl(hasName("f")));
 
   

[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov created this revision.
Herald added a subscriber: jdoerfert.
mizvekov updated this revision to Diff 374246.
mizvekov added a comment.
mizvekov updated this revision to Diff 374365.
Herald added subscribers: kbarton, nemanjai.
mizvekov updated this revision to Diff 374395.
Herald added subscribers: usaxena95, kadircet, arphaman.
mizvekov updated this revision to Diff 374401.
mizvekov updated this revision to Diff 374407.
mizvekov updated this revision to Diff 374590.
mizvekov added a comment.
Herald added a subscriber: martong.
Herald added a reviewer: shafik.
mizvekov updated this revision to Diff 374603.
mizvekov published this revision for review.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

.


mizvekov added a comment.

.


mizvekov added a comment.

..


mizvekov added a comment.

.


mizvekov added a comment.

.


mizvekov added a comment.

.


This implements the following changes:

- AutoType retains sugared deduced-as-type.
- Template argument deduction machinery analyses the sugared type all the way

down. It would previously lose the sugar on first recursion.

- Undeduced AutoType will be properly canonicalized, including the constraint

template arguments.

As a result, we start seeing sugared types in a lot more test cases,
including some which showed very unfriendly `type-parameter-*-*` types.

Depends on D110210 

Signed-off-by: Matheus Izvekov 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110216

Files:
  clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
  clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/ASTTests.cpp
  clang-tools-extra/clangd/unittests/HoverTests.cpp
  clang-tools-extra/clangd/unittests/InlayHintTests.cpp
  clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp
  clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaInit.cpp
  clang/lib/Sema/SemaLambda.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p7-cxx14.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p4-cxx14.cpp
  clang/test/CXX/over/over.match/over.match.funcs/over.match.class.deduct/p2.cpp
  clang/test/CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
  clang/test/CXX/temp/temp.decls/temp.variadic/multi-level-substitution.cpp
  clang/test/CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.call/p1-0x.cpp
  clang/test/Index/print-type.cpp
  clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp
  clang/test/SemaCXX/cxx1z-decomposition.cpp
  clang/test/SemaCXX/deduced-return-type-cxx14.cpp
  clang/test/SemaCXX/friend.cpp
  clang/test/SemaCXX/recovery-expr-type.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/sugared-auto.cpp
  clang/test/SemaTemplate/attributes.cpp
  clang/test/SemaTemplate/friend.cpp
  clang/test/SemaTemplate/operator-template.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -6176,26 +6176,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
-}
-
-TEST_P(ImportAutoFunctions, ReturnWithStructDeclaredInside2) {
-  Decl *FromTU = getTuDecl(
-  R"(
-  auto foo() {
-struct X {};
-return X();
-  }
-  )",
-  Lang_CXX14, "input0.cc");
-  FunctionDecl *From =
-  FirstDeclMatcher().match(FromTU, functionDecl());
-
-  // This time import the type directly.
-  QualType ToT = ImportType(From->getType(), From, Lang_CXX14);
-  const FunctionProtoType *FPT = cast(ToT);
-  EXPECT_TRUE(isa(FPT->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }
 
 TEST_P(ImportAutoFunctions, ReturnWithTypedefToStructDeclaredInside) {
@@ -6212,8 +6195,9 @@
   FirstDeclMatcher().match(FromTU, functionDecl());
 
   FunctionDecl *To = Import(From, Lang_CXX14);
-  EXPECT_TRUE(To);
-  EXPECT_TRUE(isa(To->getReturnType()));
+  // FIXME: We do not support importing these.
+  // EXPECT: error: cannot import unsupported AST node CXXRecord
+  EXPECT_FALSE(To);
 }