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

Enhancements for future and shared_future #3284

Merged
merged 12 commits into from
Dec 15, 2022

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Dec 12, 2022

This PR is making some enhancements for <future>.

  1. Currently std::is_nothrow_move_assignable_v<std::shared_future<void>> is false in MSVC STL (Godbolt link), which is non-conforming. This PR fixes this bug by making internal functions noexcept (inspired by Mark constructors that do not throw exceptions as noexcept #3278) and some special member functions defaulted.
  2. Currently users can touch internal constructors by definition std::future<T> ft(another_future, {}); (where another_future is of type std::future<T> or std::shared_future<T>) (Godbolt link). This PR forbids such definitions - because I think the internal constructors shouldn't be directly touched when no _Ugly name is explicitly specified, although the status quo is conforming due to [member.functions]/2.
  3. Updating the references to refer to WG21-N4917 and dropping the section number. Towards STL: Update comments citing old Working Papers #182.
  4. (Doing while reviewing) Manually inlining _Copy_from and _Move_from of _State_manager into constructors and assignment operators, and simply the codes - this is making the implementation code shorter.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner December 12, 2022 17:54
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

This generally seems like a good change to me; there are a few extra noexcepts that are needed, imo, so that the compiler doesn't generate trys.

Thanks!

stl/inc/future Show resolved Hide resolved
stl/inc/future Outdated Show resolved Hide resolved
stl/inc/future Show resolved Hide resolved
@strega-nil-ms strega-nil-ms added bug Something isn't working enhancement Something can be improved labels Dec 12, 2022
Add noexcept to _Deleter_base::_Delete, _State_deleter::_Delete, _Retain, and_Release.
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much!

stl/inc/future Outdated Show resolved Hide resolved
stl/inc/future Outdated Show resolved Hide resolved
stl/inc/future Outdated Show resolved Hide resolved
stl/inc/future Outdated Show resolved Hide resolved
In constructors we can expect that `this != _STD addressof(_Other)`, so the refactorization seems worthy.
Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

These are suggestions rather than requirements, so I'll go ahead and approve.

stl/inc/future Outdated Show resolved Hide resolved
stl/inc/future Outdated Show resolved Hide resolved
stl/inc/future Outdated Show resolved Hide resolved
Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

looks good to me!

@StephanTLavavej
Copy link
Member

Thanks - this is riskier than the average PR, but I think it preserves ABI and is a worthy improvement.

I've pushed a couple of tiny changes to the test - a trivial naming nitpick (which I wouldn't have bothered with by itself), and a line to verify that the empty-braces type trait can report both true and false, since that's a way for test coverage to be invisibly damaged. FYI @CaseyCarter @strega-nil-ms.

@StephanTLavavej StephanTLavavej self-assigned this Dec 15, 2022
@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 34b296f into microsoft:main Dec 15, 2022
@StephanTLavavej
Copy link
Member

Thanks for noticing the nonconformance here and carefully improving things! 🕵️ 😻 ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants