Skip to content

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

Merged
StephanTLavavej merged 7 commits intomicrosoft:mainfrom
frederick-vs-ja:packaged-move-only-task
Oct 12, 2024
Merged

<future>: Make packaged_task accept move-only functors#4946
StephanTLavavej merged 7 commits intomicrosoft:mainfrom
frederick-vs-ja:packaged-move-only-task

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

AlexGuteniev commented Sep 10, 2024

  • 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

AlexGuteniev commented Sep 10, 2024

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

StephanTLavavej commented Sep 21, 2024

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

StephanTLavavej commented Oct 3, 2024

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

StephanTLavavej commented Oct 11, 2024

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

StephanTLavavej commented Oct 11, 2024

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
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Oct 12, 2024

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

frederick-vs-ja commented Oct 12, 2024

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

None yet

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