Skip to content

Replace C-style cast with move() in let* algos #552

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Replace C-style cast with move() in let* algos #552

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
665e3a0
Replace C-style cast with move() in let* algos
ccotter Jul 5, 2023
69a7ff9
Remove unused state factory from operation
ccotter Jul 5, 2023
9e28159
Revert change to let_value_with and add test
ccotter Jul 6, 2023
f58d770
Merge branch 'main' of https://github.com/facebookexperimental/libuni…
ccotter Jul 6, 2023
f79acfd
One more test
ccotter Jul 6, 2023
53ebcca
More fixes
ccotter Jul 6, 2023
908df96
Fix another incorrect move
ccotter Jul 6, 2023
195503e
Update test/let_value_test.cpp
ccotter Jul 6, 2023
cc99df8
Use forward with NOLINT and comment, add verification points to tests
ccotter Jul 7, 2023
be442cd
Merge branch 'main' of https://github.com/facebookexperimental/libuni…
ccotter Jul 7, 2023
6000dfd
Merge branch 'use-move-let-algos' of https://github.com/ccotter/libun…
ccotter Jul 8, 2023
e73aaf9
typo
ccotter Jul 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/unifex/let_done.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ class _op<Source, Done, Receiver>::type {
, receiver_((Receiver2&&)dest)
{
unifex::activate_union_member_with(sourceOp_, [&] {
return unifex::connect((Source&&)source, source_receiver{this});
return unifex::connect(std::move(source), source_receiver{this});
Copy link
Contributor

@ispeters ispeters Jul 6, 2023

Choose a reason for hiding this comment

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

Somehow this has broken the MSVC build. I wonder if Source is sometimes an lvalue reference.

Copy link
Contributor

@ispeters ispeters Jul 6, 2023

Choose a reason for hiding this comment

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

Yup; line 324 instantiates an operation with an lvalue ref Source if the Sender argument is an lvalue ref. This is a std::forward.

However, most of the operation state types that I've written myself have a templated constructor that takes a forwarding reference to the Sender, rather than templating the whole type on a forwarding reference. It seems like there'd be less duplicated code in the resulting binary if we could follow that pattern.

});
startedOp_ = 0 + 1;
}
Expand Down
2 changes: 1 addition & 1 deletion include/unifex/let_error.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ class _op<Source, Func, Receiver>::type final {
: func_((Func2 &&) func)
, receiver_((Receiver2 &&) dest) {
unifex::activate_union_member_with(sourceOp_, [&] {
return unifex::connect((Source &&) source, source_receiver{this});
return unifex::connect(std::move(source), source_receiver{this});
});
}

Expand Down
7 changes: 4 additions & 3 deletions include/unifex/let_value.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <functional>
#include <tuple>
#include <type_traits>
#include <utility>

#include <unifex/detail/prologue.hpp>

Expand Down Expand Up @@ -85,7 +86,7 @@ struct _successor_receiver<Operation, Values...>::type {
void set_error(Error error) && noexcept {
auto& op = op_;
cleanup();
unifex::set_error(std::move(op.receiver_), (Error &&) error);
unifex::set_error(std::move(op.receiver_), std::move(error));
}

private:
Expand Down Expand Up @@ -178,7 +179,7 @@ struct _predecessor_receiver<Operation>::type {
void set_error(Error error) && noexcept {
auto& op = op_;
unifex::deactivate_union_member(op.predOp_);
unifex::set_error(std::move(op.receiver_), (Error &&) error);
unifex::set_error(std::move(op.receiver_), std::move(error));
}

template(typename CPO)
Expand Down Expand Up @@ -236,7 +237,7 @@ struct _op<Predecessor, SuccessorFactory, Receiver>::type {
receiver_((Receiver2 &&) receiver) {
unifex::activate_union_member_with(predOp_, [&] {
return unifex::connect(
(Predecessor &&) pred, predecessor_receiver<operation>{*this});
std::move(pred), predecessor_receiver<operation>{*this});
});
}

Expand Down
8 changes: 4 additions & 4 deletions include/unifex/let_value_with.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
#include <unifex/execution_policy.hpp>
#include <unifex/type_list.hpp>

#include <utility>

#include <unifex/detail/prologue.hpp>

namespace unifex {
Expand Down Expand Up @@ -100,20 +102,18 @@ template<typename StateFactory, typename SuccessorFactory, typename Receiver>
struct _operation<StateFactory, SuccessorFactory, Receiver>::type {
template <typename StateFactory2, typename SuccessorFactory2, typename Receiver2>
type(StateFactory2&& stateFactory, SuccessorFactory2&& func, Receiver2&& r) :
stateFactory_(static_cast<StateFactory2&&>(stateFactory)),
func_(static_cast<SuccessorFactory2&&>(func)),
state_(static_cast<StateFactory&&>(stateFactory_)()),
state_(std::forward<StateFactory2>(stateFactory)()),
innerOp_(
unifex::connect(
static_cast<SuccessorFactory&&>(func_)(state_),
std::move(func_)(state_),
static_cast<Receiver2&&>(r))) {
}

void start() & noexcept {
unifex::start(innerOp_);
}

StateFactory stateFactory_;
SuccessorFactory func_;
callable_result_t<StateFactory> state_;
connect_result_t<
Expand Down
4 changes: 3 additions & 1 deletion include/unifex/let_value_with_stop_source.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include <unifex/execution_policy.hpp>
#include <unifex/type_list.hpp>

#include <utility>

#include <unifex/detail/prologue.hpp>

namespace unifex {
Expand Down Expand Up @@ -165,7 +167,7 @@ struct _stop_source_operation<SuccessorFactory, Receiver>::type {
nothrow_connectable) {
return unifex::connect(
static_cast<SuccessorFactory&&>(func)(stopSource_),
Copy link
Contributor

@ispeters ispeters Jul 7, 2023

Choose a reason for hiding this comment

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

This should probably be a move, too, which might mean it should be taken by rvalue reference.

Suggested change
static_cast<SuccessorFactory&&>(func)(stopSource_),
std::move(func)(stopSource_),

Copy link
Contributor Author

@ccotter ccotter Jul 8, 2023

Choose a reason for hiding this comment

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

SuccessorFactory&& also seems like a "saved for later forwarding reference," although it's used differently than in the other algos. Do we need func_ to remain alive for the duration of the operation? In the operation constructor, we currently have (annotated with // comment)

template <typename SuccessorFactory2, typename Receiver2>
  type(SuccessorFactory2&& func, Receiver2&& r) noexcept(
      std::is_nothrow_constructible_v<SuccessorFactory, SuccessorFactory2>&&
          std::is_nothrow_constructible_v<Receiver, Receiver2>&& noexcept(
              connect_inner_op(func, (Receiver2 &&) r))) // borrow func, whether it's an lvalue ref or rvalue erf
    : func_{(SuccessorFactory2 &&) func} // forward func into func_
    , receiverToken_(get_stop_token(r))
    , innerOp_(connect_inner_op(func_, (Receiver2 &&) r)) {} // borrow func_, but don't move so that func_ remains alive

SuccessorFactory& in connect_inner_op seems correct so that it borrows but doesn't possibly move from.

receiver_t{*this, static_cast<Receiver&&>(r)});
receiver_t{*this, std::move(r)});
}

public:
Expand Down
2 changes: 2 additions & 0 deletions include/unifex/let_value_with_stop_token.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
#include <unifex/sender_concepts.hpp>
#include <unifex/type_list.hpp>

#include <utility>

#include <unifex/detail/prologue.hpp>

namespace unifex {
Expand Down