-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[Clang] Bypass TAD during overload resolution if a perfect match exists #136203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
…match exists This implements the same overload resolution behavior as GCC, as described in https://wg21.link/p3606 (section 1-2, not 3) If during overload resolution, there is a non-template candidate that would be always be picked - because each of the argument is a perfect match (ie the source and target types are the same), we do not perform deduction for any template candidate that might exists. The goal is to be able to merge llvm#122423 without being too disruptive. This change means that the selection of the best viable candidate and template argument deduction become interleaved. To avoid rewriting half of Clang we store in `OverloadCandidateSet` enough information to be able to deduce template candidates from `OverloadCandidateSet::BestViableFunction`. Which means the lifetime of any object used by template argument must outlive a call to `Add*Template*Candidate`. This two phase resolution is not performed for some initialization as there are cases where template candidate are better match in these cases per the standard. It's also bypassed for code completion. The change has a nice impact on compile times https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au Fixes llvm#62096 Fixes llvm#74581
… include a conversion
…tion in non-member calls
* Prefer a constructor template over a non-template conversion function
- We find a template conversion candidate - A constructor with a dependent explicit specifier
This change breaks a bunch of other cases, for example, calls to |
Please take a look, so far it seems like it should be reverted. |
Can you provide us with a reduced example? |
@alexfh I just noticed, I'll look into it (please mention people explicitly, otherwise it's unlikely people notice) |
…t perfect We might prefer a template std::initializer list constructor. Fix a regression introduced by llvm#136203 llvm#136203 (comment) GCC had a similar issue and a similar fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100963
…t perfect (#138307) We might prefer a template std::initializer list constructor. Fix a regression introduced by #136203 #136203 (comment) GCC had a similar issue and a similar fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100963
Thank you for the prompt fix! It addresses the problems we've found so far. |
This is fixed now. I think, the test case in #138307 already covers this. |
…ts (llvm#136203) This implements the same overload resolution behavior as GCC, as described in https://wg21.link/p3606 (section 1-2, not 3) If during overload resolution, there is a non-template candidate that would be always be picked - because each of the argument is a perfect match (ie the source and target types are the same), we do not perform deduction for any template candidate that might exists. The goal is to be able to merge llvm#122423 without being too disruptive. This change means that the selection of the best viable candidate and template argument deduction become interleaved. To avoid rewriting half of Clang we store in OverloadCandidateSet enough information to be able to deduce template candidates from OverloadCandidateSet::BestViableFunction. Which means the lifetime of any object used by template argument must outlive a call to Add*Template*Candidate. This two phase resolution is not performed for some initialization as there are cases where template candidate are better match in these cases per the standard. It's also bypassed for code completion. The change has a nice impact on compile times https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au Fixes llvm#62096 Fixes llvm#74581 Reapplies llvm#133426
…t perfect (llvm#138307) We might prefer a template std::initializer list constructor. Fix a regression introduced by llvm#136203 llvm#136203 (comment) GCC had a similar issue and a similar fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100963
…t perfect (llvm#138307) We might prefer a template std::initializer list constructor. Fix a regression introduced by llvm#136203 llvm#136203 (comment) GCC had a similar issue and a similar fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100963
…ts (llvm#136203) This implements the same overload resolution behavior as GCC, as described in https://wg21.link/p3606 (section 1-2, not 3) If during overload resolution, there is a non-template candidate that would be always be picked - because each of the argument is a perfect match (ie the source and target types are the same), we do not perform deduction for any template candidate that might exists. The goal is to be able to merge llvm#122423 without being too disruptive. This change means that the selection of the best viable candidate and template argument deduction become interleaved. To avoid rewriting half of Clang we store in OverloadCandidateSet enough information to be able to deduce template candidates from OverloadCandidateSet::BestViableFunction. Which means the lifetime of any object used by template argument must outlive a call to Add*Template*Candidate. This two phase resolution is not performed for some initialization as there are cases where template candidate are better match in these cases per the standard. It's also bypassed for code completion. The change has a nice impact on compile times https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au Fixes llvm#62096 Fixes llvm#74581 Reapplies llvm#133426
…t perfect (llvm#138307) We might prefer a template std::initializer list constructor. Fix a regression introduced by llvm#136203 llvm#136203 (comment) GCC had a similar issue and a similar fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100963
…ed-init-list perfect (#138307) We might prefer a template std::initializer list constructor. Fix a regression introduced by #136203 llvm/llvm-project#136203 (comment) GCC had a similar issue and a similar fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100963
…t perfect (llvm#138307) We might prefer a template std::initializer list constructor. Fix a regression introduced by llvm#136203 llvm#136203 (comment) GCC had a similar issue and a similar fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100963
I was confused a little here. It sounded like this patch will make strictly more programs accepted. However, this seems not true: https://godbolt.org/z/erhjKhKP5 This program might be problematic to accept in the first place, which means that this patch has some side effect to indirectly fixing other bugs. (e.g. 1. why removing unused field in base class made clang 20 reject the program. and 2. why remove the subclass made clang 20 reject the program) |
…nique_ptr<T>> Summary: Adding this as a test for the new compiler. With this upstream PR: llvm/llvm-project#136203 This test starts to fail with the following error: ``` fbcode/folly/memory/test/not_null_test.cpp:465:24: error: call to constructor of 'std::unique_ptr<int>' is ambiguous 465 | std::unique_ptr<int> i{std::move(nnu)}; | ^~~~~~~~~~~~~~~~~ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:327:7: note: candidate constructor 327 | unique_ptr(unique_ptr&&) = default; | ^ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:468:7: note: candidate constructor has been explicitly deleted 468 | unique_ptr(const unique_ptr&) = delete; | ^ fbcode/folly/memory/test/not_null_test.cpp:467:7: error: static assertion failed due to requirement 'std::is_constructible_v<std::unique_ptr<int, std::default_delete<int>>, folly::not_null<std::unique_ptr<int, std::default_delete<int>>>>' 467 | std::is_constructible_v< | ^~~~~~~~~~~~~~~~~~~~~~~~ 468 | std::unique_ptr<int>, | ~~~~~~~~~~~~~~~~~~~~~ 469 | not_null<std::unique_ptr<int>>>); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated. ``` Note that when you construct a unique_ptr, its deleted copy constructor still participates in overload resolution. So it's possible for a operator const PtrT&() const& noexcept to be selected. Meanwhile, its operator PtrT&&() && noexcept; is selected to convert not_null to the unique_ptr&& for the move constructor. Hence the ambiguity. Why is this only a problem now? See D74599633. The existence of the conversion operator template was a better match to create a unique_ptr from a not_null previously. However, the SFINAE conditions are circular and caused weird compiler behaviours. This upstream PR now stops looking at templates when there's a "perfect match", which are the nontemplated `operator PtrT&&` and `operator const PtrT&` for not_null. It's possible that the PR has hidden some bugs, but the behaviour is unspecified by the standard anyway. The PR implements the change to make it align with GCC behaviour. Reviewed By: snarkmaster, Gownta Differential Revision: D74602687 fbshipit-source-id: ad6c49e6fa57c6340d6646734e9b6120e309353c
…nique_ptr<T>> Summary: Adding this as a test for the new compiler. With this upstream PR: llvm/llvm-project#136203 This test starts to fail with the following error: ``` fbcode/folly/memory/test/not_null_test.cpp:465:24: error: call to constructor of 'std::unique_ptr<int>' is ambiguous 465 | std::unique_ptr<int> i{std::move(nnu)}; | ^~~~~~~~~~~~~~~~~ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:327:7: note: candidate constructor 327 | unique_ptr(unique_ptr&&) = default; | ^ fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/unique_ptr.h:468:7: note: candidate constructor has been explicitly deleted 468 | unique_ptr(const unique_ptr&) = delete; | ^ fbcode/folly/memory/test/not_null_test.cpp:467:7: error: static assertion failed due to requirement 'std::is_constructible_v<std::unique_ptr<int, std::default_delete<int>>, folly::not_null<std::unique_ptr<int, std::default_delete<int>>>>' 467 | std::is_constructible_v< | ^~~~~~~~~~~~~~~~~~~~~~~~ 468 | std::unique_ptr<int>, | ~~~~~~~~~~~~~~~~~~~~~ 469 | not_null<std::unique_ptr<int>>>); | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated. ``` Note that when you construct a unique_ptr, its deleted copy constructor still participates in overload resolution. So it's possible for a operator const PtrT&() const& noexcept to be selected. Meanwhile, its operator PtrT&&() && noexcept; is selected to convert not_null to the unique_ptr&& for the move constructor. Hence the ambiguity. Why is this only a problem now? See D74599633. The existence of the conversion operator template was a better match to create a unique_ptr from a not_null previously. However, the SFINAE conditions are circular and caused weird compiler behaviours. This upstream PR now stops looking at templates when there's a "perfect match", which are the nontemplated `operator PtrT&&` and `operator const PtrT&` for not_null. It's possible that the PR has hidden some bugs, but the behaviour is unspecified by the standard anyway. The PR implements the change to make it align with GCC behaviour. Reviewed By: snarkmaster, Gownta Differential Revision: D74602687 fbshipit-source-id: ad6c49e6fa57c6340d6646734e9b6120e309353c
@cor3ntin we've root-caused an assertion failure in Clang to this commit. It reproduces at least up to 1778d3b. The test case is being reduced, but maybe you can spot something obviously wrong from the assertion failure and the stack trace:
And the stack trace dump from lldb (llvm revision 1778d3b):
|
@alexfh Unfortunately, I will need a repro. I'm very curious to see what case we forgot to account for. Thanks in advance! |
Reduced test case: https://gcc.godbolt.org/z/qG1Yv35rb |
@cor3ntin ^^ |
Thanks, I will look into it today |
Function pointers can have an identity conversion to a pointer to member function if they are resolved to a member function. Fix a regression introduced by llvm#136203
Function pointers can have an identity conversion to a pointer to member function if they are resolved to a member function. Fix a regression introduced by llvm#136203
Function pointers can have an identity conversion to a pointer to member function if they are resolved to a member function. Fix a regression introduced by #136203
This implements the same overload resolution behavior as GCC,
as described in https://wg21.link/p3606 (section 1-2, not 3)
If during overload resolution, there is a non-template candidate
that would be always be picked - because each of the argument
is a perfect match (ie the source and target types are the same),
we do not perform deduction for any template candidate
that might exists.
The goal is to be able to merge #122423 without being too disruptive.
This change means that the selection of the best viable candidate and
template argument deduction become interleaved.
To avoid rewriting half of Clang we store in OverloadCandidateSet
enough information to be able to deduce template candidates from
OverloadCandidateSet::BestViableFunction. Which means
the lifetime of any object used by template argument must outlive
a call to AddTemplateCandidate.
This two phase resolution is not performed for some initialization
as there are cases where template candidate are better match
in these cases per the standard. It's also bypassed for code completion.
The change has a nice impact on compile times
https://llvm-compile-time-tracker.com/compare.php?from=719b029c16eeb1035da522fd641dfcc4cee6be74&to=bf7041045c9408490c395230047c5461de72fc39&stat=instructions%3Au
Fixes #62096
Fixes #74581
Reapplies #133426