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

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

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Sep 10, 2024

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.

@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
stl/inc/future Outdated Show resolved Hide resolved
@CaseyCarter

This comment was marked as off-topic.

stl/inc/functional Outdated Show resolved Hide resolved
@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 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.

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
Copy link
Contributor Author

I think it worth creating vNext issue.

Or maybe TRANSITION, ABI comment(s)

#5009 is created for tracking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

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