`<future>` cleanups #3940

achabense · 2023-08-09T13:18:19Z

replaces long trailing decltype return types with decltype(auto) in several places
removes some redundant _Packaged_state constructors
simplifies signatures of _Packaged_state::_Get_fn, and remove an extra copy construction around it
makes some usings private in packaged_task
removes some uncommon forward patterns

…) then ctor(&)` now `ctor(const&)`)

achabense · 2023-08-09T13:20:21Z

stl/inc/future

-    _Packaged_state(const _Fty2& _Fnarg, const _Alloc& _Al, _Mydel* _Dp)
-        : _Mybase(_Dp), _Fn(allocator_arg, _Al, _Fnarg) {}
-#endif // _HAS_FUNCTION_ALLOCATOR_SUPPORT
-


They are fully replaceable with their _Fty2&& versions

achabense · 2023-08-09T13:22:21Z

stl/inc/future

@@ -1322,8 +1322,7 @@ public:
    void reset() { // reset to newly constructed state
        _MyStateManagerType& _State         = _MyPromise._Get_state_for_set();
        _MyStateType* _MyState              = static_cast<_MyStateType*>(_State._Ptr());
-        function<_Ret(_ArgTypes...)> _Fnarg = _MyState->_Get_fn();
-        _MyPromiseType _New_promise(new _MyStateType(_Fnarg));
+        _MyPromiseType _New_promise(new _MyStateType(_MyState->_Get_fn()));


I'm 80% sure about this change; or was the extra copy construction intentional here?

I don't believe it was intentional.

StephanTLavavej · 2023-08-09T17:26:57Z

Can you please provide PR descriptions so that reviewers, and future code archaeologists, can quickly understand what is being changed and why? Only for something completely trivial like "Fix comment typos" is a PR title sufficient.

achabense · 2023-08-10T01:06:03Z

Hmm, added.

frederick-vs-ja · 2023-08-10T10:03:12Z

STL/stl/inc/future

Line 1403 in ed8150e

// update, so this adapter turns copies into terminate(). When VSO-153581 is

Now we should say abort() instead. Should we change the comment in this PR or file a "good first issue"?

StephanTLavavej · 2023-08-10T18:58:04Z

I'm in favor of just fixing the comment here. "Good first issues" are a bit more work for the maintainers and we don't have a lot of capacity right now.

achabense · 2023-09-04T08:16:14Z

I think the usage of forward looks strange here. Can we replace it with move or static_cast<_Mybase&&>? (also at line 988 and 1015)

STL/stl/inc/future

Lines 962 to 963 in 6c69a73

    
           shared_future(future<_Ty>&& _Other) noexcept : _Mybase(_STD forward<_Mybase>(_Other)) {}

CaseyCarter · 2023-09-05T19:50:27Z

I think the usage of forward looks strange here. Can we replace it with move or static_cast<_Mybase&&>?

I'm not bothered by this usage of forward because I've heard Howard Hinnant describe why it's an intended usage. That said, it certainly never caught on and I don't consider it idiomatic. I would much rather have static_cast<_Mybase&&> here than std::move if we're going to make a change: I like it explicit that there's something weird happening here.

achabense · 2023-09-06T07:58:43Z

stl/inc/future

@@ -1384,7 +1384,7 @@ public:

    [[noreturn]] _Fake_no_copy_callable_adapter(const _Fake_no_copy_callable_adapter& _Other)
        : _Storage(_STD move(_Other._Storage)) {
-        _CSTD abort(); // shouldn't be called, see GH-3888
+        _CSTD abort(); // shouldn't be called


( I begin to feel "see GH-3888" a bit awkward, as the reason has been explained at the beginning of the class :| )

StephanTLavavej · 2023-09-15T00:58:23Z

stl/inc/future

@@ -1322,8 +1322,7 @@ public:
    void reset() { // reset to newly constructed state
        _MyStateManagerType& _State         = _MyPromise._Get_state_for_set();
        _MyStateType* _MyState              = static_cast<_MyStateType*>(_State._Ptr());
-        function<_Ret(_ArgTypes...)> _Fnarg = _MyState->_Get_fn();
-        _MyPromiseType _New_promise(new _MyStateType(_Fnarg));
+        _MyPromiseType _New_promise(new _MyStateType(_MyState->_Get_fn()));


I don't believe it was intentional.

StephanTLavavej · 2023-09-15T01:00:43Z

stl/inc/future

-auto _Invoke_stored(tuple<_Types...>&& _Tuple)
-    -> decltype(_Invoke_stored_explicit(_STD move(_Tuple), index_sequence_for<_Types...>{})) { // invoke() a tuple
+decltype(auto) _Invoke_stored(tuple<_Types...>&& _Tuple) { // invoke() a tuple


No change requested: The difference here is that decltype(auto) won't SFINAE, but it appears that we don't need SFINAE here (nothing can directly ask is_invocable).

StephanTLavavej · 2023-09-20T23:36:04Z

I think we can merge this without a second maintainer approval.

StephanTLavavej · 2023-09-20T23:55:13Z

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

StephanTLavavej · 2023-09-21T19:41:29Z

⏳ ⏱️ 🧹

achabense added 4 commits August 9, 2023 20:26

replace auto->long expr with decltype(auto)

86b16ce

_Packaged_state(const auto&) ctors are useless

f8b81b0

around _Get_fn; remove an extra copy construction (was `ctor(const&…

8cf4877

…) then ctor(&)` now `ctor(const&)`)

hide some typedefs in packaged_task

f6fc01f

achabense requested a review from a team as a code owner August 9, 2023 13:18

github-actions bot added this to Initial Review in Code Reviews Aug 9, 2023

achabense changed the title ~~<future> cleanups~~ <future> cleanups Aug 9, 2023

achabense commented Aug 9, 2023

View reviewed changes

StephanTLavavej added the enhancement Something can be improved label Aug 9, 2023

StephanTLavavej self-assigned this Aug 9, 2023

comment fix

e8b528c

achabense added 2 commits September 6, 2023 15:51

replace uncommon forward patterns

9327f04

nitpick on my previous pr

afd9d6f

achabense commented Sep 6, 2023

View reviewed changes

StephanTLavavej approved these changes Sep 15, 2023

View reviewed changes

StephanTLavavej removed their assignment Sep 15, 2023

StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Sep 15, 2023

StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 20, 2023

StephanTLavavej self-assigned this Sep 20, 2023

StephanTLavavej merged commit 4f97dbb into microsoft:main Sep 21, 2023
35 checks passed

Code Reviews automation moved this from Ready To Merge to Done Sep 21, 2023

achabense deleted the _Future_cleanups branch September 22, 2023 03:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`<future>` cleanups #3940

`<future>` cleanups #3940

achabense commented Aug 9, 2023 •

edited

achabense Aug 9, 2023

achabense Aug 9, 2023

StephanTLavavej Sep 15, 2023

StephanTLavavej commented Aug 9, 2023

achabense commented Aug 10, 2023 •

edited

frederick-vs-ja commented Aug 10, 2023

StephanTLavavej commented Aug 10, 2023

achabense commented Sep 4, 2023 •

edited

CaseyCarter commented Sep 5, 2023

achabense Sep 6, 2023 •

edited

StephanTLavavej Sep 15, 2023

StephanTLavavej Sep 15, 2023

StephanTLavavej commented Sep 20, 2023

StephanTLavavej commented Sep 20, 2023

StephanTLavavej commented Sep 21, 2023

<future> cleanups #3940

<future> cleanups #3940

Conversation

achabense commented Aug 9, 2023 • edited

achabense Aug 9, 2023

Choose a reason for hiding this comment

achabense Aug 9, 2023

Choose a reason for hiding this comment

StephanTLavavej Sep 15, 2023

Choose a reason for hiding this comment

StephanTLavavej commented Aug 9, 2023

achabense commented Aug 10, 2023 • edited

frederick-vs-ja commented Aug 10, 2023

StephanTLavavej commented Aug 10, 2023

achabense commented Sep 4, 2023 • edited

CaseyCarter commented Sep 5, 2023

achabense Sep 6, 2023 • edited

Choose a reason for hiding this comment

StephanTLavavej Sep 15, 2023

Choose a reason for hiding this comment

StephanTLavavej Sep 15, 2023

Choose a reason for hiding this comment

StephanTLavavej commented Sep 20, 2023

StephanTLavavej commented Sep 20, 2023

StephanTLavavej commented Sep 21, 2023

⏳ ⏱️ 🧹

`<future>` cleanups #3940

`<future>` cleanups #3940

achabense commented Aug 9, 2023 •

edited

achabense commented Aug 10, 2023 •

edited

achabense commented Sep 4, 2023 •

edited

achabense Sep 6, 2023 •

edited