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

Explicitly spell the noexcept-specifier of the move constructor and move assignment operator of packaged_task #4940

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

heckerpowered
Copy link
Contributor

https://en.cppreference.com/w/cpp/thread/packaged_task/packaged_task
https://en.cppreference.com/w/cpp/thread/packaged_task/operator%3D

Cppref states that the move constructor and move assignment operator of packaged_task are noexcept, so I think there should be a noexcept specifier here.

@heckerpowered heckerpowered requested a review from a team as a code owner September 6, 2024 15:48
@frederick-vs-ja
Copy link
Contributor

This PR seems to be an improvement as it makes noexcept specified by the Standard explicitly spelt.

However, the touched move functions are already implicitly noexcept. E.g. the following code snippet is accepted with MSVC STL (Godbolt link).

#include <future>
#include <type_traits>

static_assert(std::is_nothrow_move_constructible_v<std::packaged_task<void()>>);
static_assert(std::is_nothrow_move_constructible_v<std::packaged_task<int(long)>>);

static_assert(std::is_nothrow_move_assignable_v<std::packaged_task<void()>>);
static_assert(std::is_nothrow_move_assignable_v<std::packaged_task<int(long)>>);

I think the title of this PR should be changed to

Explicitly spell the noexcept-specifier of the move constructor and move assignment operator of packaged_task

@heckerpowered heckerpowered changed the title Add noexcept specifier to move constructor and move assignment operator Explicitly spell the noexcept-specifier of the move constructor and move assignment operator of packaged_task Sep 6, 2024
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Sep 6, 2024
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

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

Oops, the original explicit noexcept-specifiers were dropped by #3315... by me.

I used to think that implicit exception specification should be sufficient and would slightly speed up compilation, but I ignored the drawback that it would be harder for users to confirm and rely on the fact that these move functions are always noexcept.

@heckerpowered
Copy link
Contributor Author

@microsoft-github-policy-service agree

@CaseyCarter
Copy link
Member

CaseyCarter commented Sep 6, 2024

Oops, the original explicit noexcept-specifiers were dropped by #3315... by me.

It's not all on you: in the past we've avoided explicit noexcepts - and even some constexprs - on defaulted member functions. I think it was just to have less code to read, both for us and (as you suggest) the compiler.

I agree that we should be explicit given the enormous number of readers the code gets with a diversity of experiences. It would be nice not to have to read the entire header simply to determine that packaged_task's move constructor is noexcept, or example.

@CaseyCarter
Copy link
Member

image

@heckerpowered, for future reference we prefer that authors don't force push PR branches after review starts because it tends to break incremental code review. (GitHub's "show changes since your last review" feature gets confused. Or at least it did, I haven't checked for a while.) It's obviously a non-issue for this two-line change, but for bigger things we will cry and gnash our teeth with frustration. And nobody likes gnashing.

@StephanTLavavej
Copy link
Member

I remembered to check for R& and void specializations (as seen in other <future> technology) but packaged_task doesn't have them, so this change is correct and complete. 😸

@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 e067e3e into microsoft:main Sep 9, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for this clarity improvement and congratulations on your first microsoft/STL commit! 🎉 😸 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants