Skip to content
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

LWG-2774 std::function construction vs assignment #2098

Merged
merged 12 commits into from
Dec 17, 2021
Merged

LWG-2774 std::function construction vs assignment #2098

merged 12 commits into from
Dec 17, 2021

Conversation

sam20908
Copy link
Contributor

@sam20908 sam20908 commented Aug 6, 2021

Partially addresses #1965

@sam20908 sam20908 requested a review from a team as a code owner August 6, 2021 01:47
@sam20908 sam20908 marked this pull request as draft August 6, 2021 02:37
@StephanTLavavej StephanTLavavej added the LWG Library Working Group issue label Aug 6, 2021
@sam20908 sam20908 marked this pull request as ready for review August 6, 2021 03:30
@sam20908 sam20908 marked this pull request as draft August 6, 2021 13:36
@sam20908 sam20908 marked this pull request as ready for review August 6, 2021 14:20
Copy link
Contributor

@fsb4000 fsb4000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compile-only tests should be test.compile.pass.cpp

Copy link
Contributor

@fsb4000 fsb4000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also edit the file: https://github.com/Microsoft/STL/blob/main/tests/std/test.lst
(add tests\LWG2774_function_construction_vs_assignment)

@sam20908
Copy link
Contributor Author

sam20908 commented Aug 6, 2021

It appears that the following somehow became true when it's supposed to be false:

static_assert(is_constructible_v<decay_t<_Fx>, _Fx>, "decay_t<F> shall be constructible with F");

However, if I pass in a Fn instance instead (I added Fn() = default), then the assertion will pick it up:

void test() {
    const Fn fn;
    function<void()> f{fn}; // fail to compile here because of the assertion
}

I am unsure how this can be...

EDIT: Oh, maybe since the compiler technically found a candidate (not at the assertion yet), is_constructible_v becomes true?

@fsb4000
Copy link
Contributor

fsb4000 commented Aug 6, 2021

yes, you are correct:
https://gcc.godbolt.org/z/zjqvTc38W

@sam20908 sam20908 marked this pull request as draft August 6, 2021 17:18
@sam20908
Copy link
Contributor Author

sam20908 commented Aug 6, 2021

I am making this a draft right now as there seems to be no way of testing a valid instantiation with failed assertion (the detector pattern implemented is currently malfunctional too).

@sam20908 sam20908 marked this pull request as ready for review August 6, 2021 18:00
Copy link
Contributor

@fsb4000 fsb4000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It looks like we really can't create tests for that.

@StephanTLavavej StephanTLavavej self-assigned this Aug 11, 2021
stl/inc/functional Outdated Show resolved Hide resolved
stl/inc/functional Outdated Show resolved Hide resolved
stl/inc/functional Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Dec 9, 2021
@StephanTLavavej
Copy link
Member

Thanks - and sorry again for taking so long to get to this 😿. Your changes are completely correct as far as I can tell 😻, so to move this forward to Final Review I've pushed a merge with main (no conflicts, but there have been a lot of commits in main including submodule changes so it was time), plus changes for a couple of minor codebase convention issues, and a SFINAE cleanup to match the new Standardese.

@StephanTLavavej StephanTLavavej self-assigned this Dec 16, 2021
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 990576e into microsoft:main Dec 17, 2021
@StephanTLavavej
Copy link
Member

Thanks for improving std::function, one of the STL's most popular types! 🚀 🎉 😸

@sam20908 sam20908 deleted the lwg_2774 branch December 17, 2021 04:10
CaseyCarter added a commit to CaseyCarter/mongo that referenced this pull request Dec 23, 2021
…ven after LWG-2774

[LWG-2774](https://wg21.link/lwg2774) changes `std::function`'s converting constructor to accept its argument by forwarding reference. With that change implemented, that constructor becomes a better match for non-`const` `unique_function` arguments than `unique_function`'s `const`-qualified deleted conversion operator template to `std::function`. As a result, `std::is_convertible_v<unique_function<T>, std::function<T>>` changes from `false` to `true` when Standard Libraries implement LWG-2774. (MSVC recently implemented this LWG issue in microsoft/STL#2098 and noticed the `unique_function` test failing as a result.)

I believe the fix is to add another deleted conversion operator template that is not `const`-qualified to `unique_function`. This makes the test pass locally, as well as correcting a simplified test case with GCC trunk (microsoft/STL#2098) which I believe also implements LWG-2774.
evrg-bot-webhook pushed a commit to mongodb/mongo that referenced this pull request Apr 4, 2022
…::function` even after LWG-2774

[LWG-2774](https://wg21.link/lwg2774) changes `std::function`'s converting constructor to accept its argument by forwarding reference. With that change implemented, that constructor becomes a better match for non-`const` `unique_function` arguments than `unique_function`'s `const`-qualified deleted conversion operator template to `std::function`. As a result, `std::is_convertible_v<unique_function<T>, std::function<T>>` changes from `false` to `true` when Standard Libraries implement LWG-2774. (MSVC recently implemented this LWG issue in microsoft/STL#2098 and noticed the `unique_function` test failing as a result.)

I believe the fix is to add another deleted conversion operator template that is not `const`-qualified to `unique_function`. This makes the test pass locally, as well as correcting a simplified test case with GCC trunk (microsoft/STL#2098) which I believe also implements LWG-2774.

Closes #1437

Signed-off-by: Blake Oler <[email protected]> and Billy Donahue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LWG Library Working Group issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants