KEMBAR78
`<future>`: Make `packaged_task` accept move-only functors by frederick-vs-ja · Pull Request #4946 · microsoft/STL · GitHub
Skip to content

<future>: Make packaged_task accept move-only functors #4946

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 12, 2024
Merged

<future>: Make packaged_task accept move-only functors #4946

merged 7 commits into from
Oct 12, 2024

Conversation

@frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Sep 10, 2024 •
edited
Loading

Fixes #321.

The issue was considered ABI-breaking, but I think it can be resolved in an ABI-preserving way like #2568. This PR adds internal constructors to function to accept non-copy-constructible functors and add static_assert to keep the copyability checking for standard constructors. Valid user codes won't be able to call these internal constructors.

Also implements the previously missing Mandates in [futures.task.members]/3, and switches to use move construction in reset per [futures.task.members]/26.

Notes:

  • The internal constructors are made constrained templates to avoid affecting overload resolution unexpectedly, see LLVM-103409.
  • The involved allocator-extended constructors were removed in C++17 by WG21-P0302R1 and LWG-2921, so this PR cites WG21-N4140 in the C++17-removed constructors.
  • The fix should work for most move-only types. Although there can be classes whose copy constructor are only invalid in instantation. In vNext we should completely get rid of function.
  • If the function<R(Args...)> is a program-defined specialization, this approach doesn't work. I don't think any user should specialize std::function, but this is allowed by the standard.
  • Three _Packaged_state specializations are merged into one. I believe this can ease maintenance.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner September 10, 2024 05:48
@AlexGuteniev
Copy link
Contributor

  • In vNext we should completely get rid of function.

I think it worth creating vNext issue.

move_only_function might be helpful for vNext implementation.

frederick-vs-ja
frederick-vs-ja commented Sep 10, 2024
View reviewed changes
@AlexGuteniev
Copy link
Contributor

I think it worth creating vNext issue.

Or maybe TRANSITION, ABI comment(s)

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 10, 2024
@StephanTLavavej StephanTLavavej self-assigned this Sep 10, 2024
frederick-vs-ja
frederick-vs-ja commented Sep 10, 2024
View reviewed changes
@CaseyCarter

This comment was marked as off-topic.

Sign in to view
AlexGuteniev
AlexGuteniev reviewed Sep 12, 2024
View reviewed changes
@jwakely
Copy link

jwakely commented Sep 19, 2024

  • The involved allocator-extended constructors were removed in C++17 by WG21-P0302R1 and LWG-2921, so this PR cites WG21-N4140 in the C++17-removed constructors.

N.B. I'm proposing to restore packaged_task(allocator_arg_t, const Allocator&, F&&)

@jwakely
Copy link

jwakely commented Sep 20, 2024

N.B. I'm proposing to restore packaged_task(allocator_arg_t, const Allocator&, F&&)

See LWG3003.

And I've just created LWG4158 which is also about packaged_task.

@StephanTLavavej
Copy link
Member

Thanks @jwakely! BTW, in this repo's issues you can say LWG-4158 with a dash to use the autolink syntax we've set up - no need to manually craft hyperlinks. (https://github.com/microsoft/STL/wiki/Custom-Autolinks has the full list.)

@StephanTLavavej
Copy link
Member

Thanks for the extremely careful work! 😻

@StephanTLavavej StephanTLavavej removed their assignment Oct 3, 2024
@StephanTLavavej StephanTLavavej mentioned this pull request Oct 3, 2024
Open
@StephanTLavavej StephanTLavavej self-assigned this Oct 11, 2024
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej
Perma-workaround VSO-2279389.
fcadb1d 
VSO-2279389 "/clr C++20 can't handle struct MoveOnlyFunctor defined in a function template, emitting fatal error C1193: an error expected in yyaction.cpp(2899) not reached"
@StephanTLavavej
Copy link
Member

I pushed a commit to perma-workaround VSO-2279389 "/clr C++20 can't handle struct MoveOnlyFunctor defined in a function template, emitting fatal error C1193: an error expected in yyaction.cpp(2899) not reached". Moving it out of the function template is sufficient and reasonable to do permanently.

@StephanTLavavej StephanTLavavej merged commit d08e31c into microsoft:main Oct 12, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for fixing this impossible bug! 🐞 ✅ 😻

@frederick-vs-ja frederick-vs-ja deleted the packaged-move-only-task branch October 12, 2024 06:36
@frederick-vs-ja frederick-vs-ja mentioned this pull request Oct 12, 2024
Open
@frederick-vs-ja
Copy link
Contributor Author

I think it worth creating vNext issue.

Or maybe TRANSITION, ABI comment(s)

#5009 is created for tracking.

@frederick-vs-ja frederick-vs-ja mentioned this pull request Nov 24, 2024
Closed
34 tasks
@StephanTLavavej StephanTLavavej mentioned this pull request Nov 24, 2024
Closed
@StephanTLavavej StephanTLavavej mentioned this pull request Oct 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+3 more reviewers

@CaseyCarter CaseyCarter CaseyCarter left review comments

@jwakely jwakely jwakely left review comments

@AlexGuteniev AlexGuteniev AlexGuteniev left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working

Projects

STL Code Reviews
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<future>: packaged_task can't be constructed from a move-only lambda

5 participants

@frederick-vs-ja @AlexGuteniev @CaseyCarter @jwakely @StephanTLavavej