Explicitly spell the noexcept-specifier of the move constructor and move assignment operator of packaged_task by heckerpowered · Pull Request #4940 · microsoft/STL (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation8 Commits1 Checks39 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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 #include

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 changed the titleAdd 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

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.

@microsoft-github-policy-service agree

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.

@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.

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. 😸

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

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