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.

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

Update finally to support value channel references #545

Merged
merged 7 commits into from
Jul 5, 2023
Merged

Update finally to support value channel references #545

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9feb969
Update finally to support value channel references
ccotter Jun 24, 2023
472130c
Address some feedback
ccotter Jul 3, 2023
2bb4f83
Use move()
ccotter Jul 3, 2023
6a722c6
Fix conversion to always create a new object for create<T>
ccotter Jul 3, 2023
e2453cc
Apply suggestions from code review
ccotter Jul 4, 2023
ab1811d
Use explicit cast from forwarded Ts... to ValueTypes
ccotter Jul 4, 2023
913eaf6
Compare pointers
ccotter Jul 4, 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
50 changes: 39 additions & 11 deletions include/unifex/create.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,26 @@
namespace unifex {
namespace _create {

template <typename Receiver, typename Fn, typename Context>
template <typename From, typename To, typename = void>
struct _do_convert {
To operator()(From&&) = delete;
};

template <typename From>
struct _do_convert<From, From, void> {
From&& operator()(From&& obj) noexcept {
return static_cast<From&&>(obj);
}
};

template <typename From, typename To>
struct _do_convert<From, To, std::void_t<std::enable_if_t<convertible_to<From, To>>>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the void_t is redundant here; I would expect enable_if_t to already alias void when the condition is true.

Suggested change
struct _do_convert<From, To, std::void_t<std::enable_if_t<convertible_to<From, To>>>> {
struct _do_convert<From, To, std::enable_if_t<convertible_to<From, To>>> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code ran into ambiguous template error when From=To (e.g., do_convert<int&,int&>), and void_t appeared to make this template more specialization, but to be honest, I am not great with SFINAE in this case. What did end up working was std::enable_if_t<!std::is_same_v<From, To> && convertible_to<From, To>> without the void_t.

Unsurprisingly, a C++20 constraints version worked with just template <typename From, typename To> requires convertible_to<From, To> struct _do_convert<From, To> { ...

In any case, I don't think we need _do_convert.

To operator()(From&& obj) {
return static_cast<From&&>(obj);
}
};

template <typename Receiver, typename Fn, typename Context, typename... ValueTypes>
struct _op {
struct type {
explicit type(Receiver rec, Fn fn, Context ctx)
Expand All @@ -34,9 +53,18 @@ struct _op {
: rec_((Receiver&&) rec), fn_((Fn&&) fn), ctx_((Context&&) ctx) {}

template (typename... Ts)
(requires receiver_of<Receiver, Ts...>)
void set_value(Ts&&... ts) noexcept(is_nothrow_receiver_of_v<Receiver, Ts...>) {
unifex::set_value((Receiver&&) rec_, (Ts&&) ts...);
(requires receiver_of<Receiver, Ts...> AND (convertible_to<Ts, ValueTypes> && ...))
ccotter marked this conversation as resolved.
Show resolved Hide resolved
void set_value(Ts&&... ts) noexcept {
UNIFEX_TRY {
// Satisfy the value completion contract with _do_convert, which
// ensures the value passed to the receiver conforms to
// the value types of the create sender. For example, if set_value
// is called with an lvalue reference but create sends non-reference
// values, _do_convert decays the provided Ts.
unifex::set_value(std::move(rec_), _do_convert<Ts&&, ValueTypes>{}(static_cast<Ts&&>(ts))...);
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we've agreed on an implementation of _do_convert and what type parameters to pass to it, is this any different?

Suggested change
unifex::set_value(std::move(rec_), _do_convert<Ts&&, ValueTypes>{}(static_cast<Ts&&>(ts))...);
unifex::set_value(std::move(rec_), ValueTypes{static_cast<Ts&&>(ts)}...);

If it's the same behaviour, do you see value in the different spelling? I might be convinced that the _do_convert version makes the intention clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, static_cast<ValueTypes>(static_cast<Ts&&>(ts)...) works but the ValueTypes{...}does not (I get -Wc++11-narrowing with the int -> double conversion test, and the create<A> test fails since set_value<B> tries to call A's aggregate initialization with a B which does not work (I think that's what is going on at least). B is convertible to A via the operator A() const member of B.

} UNIFEX_CATCH(...) {
unifex::set_error(std::move(rec_), std::current_exception());
}
}

template (typename Error)
Expand Down Expand Up @@ -79,10 +107,10 @@ struct _op {
};
};

template <typename Receiver, typename Fn, typename Context>
using _operation = typename _op<Receiver, Fn, Context>::type;
template <typename Receiver, typename Fn, typename Context, typename... ValueTypes>
using _operation = typename _op<Receiver, Fn, Context, ValueTypes...>::type;

template <typename Fn, typename Context>
template <typename Fn, typename Context, typename... ValueTypes>
struct _snd_base {
struct type {
template <template<typename...> class Variant>
Expand All @@ -101,14 +129,14 @@ struct _snd_base {
(requires derived_from<remove_cvref_t<Self>, type> AND
constructible_from<Fn, member_t<Self, Fn>> AND
constructible_from<Context, member_t<Self, Context>>)
ccotter marked this conversation as resolved.
Show resolved Hide resolved
friend _operation<remove_cvref_t<Receiver>, Fn, Context>
friend _operation<remove_cvref_t<Receiver>, Fn, Context, ValueTypes...>
tag_invoke(tag_t<connect>, Self&& self, Receiver&& rec)
noexcept(std::is_nothrow_constructible_v<
_operation<Receiver, Fn, Context>,
_operation<Receiver, Fn, Context, ValueTypes...>,
Receiver,
member_t<Self, Fn>,
member_t<Self, Context>>) {
return _operation<remove_cvref_t<Receiver>, Fn, Context>{
return _operation<remove_cvref_t<Receiver>, Fn, Context, ValueTypes...>{
(Receiver&&) rec,
((Self&&) self).fn_,
((Self&&) self).ctx_};
Expand All @@ -121,7 +149,7 @@ struct _snd_base {

template <typename Fn, typename Context, typename... ValueTypes>
struct _snd {
struct type : _snd_base<Fn, Context>::type {
struct type : _snd_base<Fn, Context, ValueTypes...>::type {
template <template<typename...> class Variant, template <typename...> class Tuple>
using value_types = Variant<Tuple<ValueTypes...>>;
};
Expand Down
11 changes: 5 additions & 6 deletions include/unifex/finally.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ namespace unifex
SourceSender,
CompletionSender,
Receiver,
std::decay_t<Values>...>::type;
Values...>::type;

template <
typename SourceSender,
Expand Down Expand Up @@ -384,7 +384,7 @@ namespace unifex
auto* const op = op_;

UNIFEX_TRY {
unifex::activate_union_member<std::tuple<std::decay_t<Values>...>>(
unifex::activate_union_member<std::tuple<Values...>>(
op->value_, static_cast<Values&&>(values)...);
} UNIFEX_CATCH (...) {
std::move(*this).set_error(std::current_exception());
Expand All @@ -411,8 +411,7 @@ namespace unifex
});
unifex::start(completionOp);
} UNIFEX_CATCH (...) {
using decayed_tuple_t = std::tuple<std::decay_t<Values>...>;
unifex::deactivate_union_member<decayed_tuple_t>(op->value_);
unifex::deactivate_union_member<std::tuple<Values...>>(op->value_);
unifex::set_error(
static_cast<Receiver&&>(op->receiver_), std::current_exception());
}
Expand Down Expand Up @@ -593,7 +592,7 @@ namespace unifex
sender_value_types_t<
remove_cvref_t<SourceSender>,
manual_lifetime_union,
decayed_tuple<std::tuple>::template apply>
std::tuple>
value_;
};

Expand Down Expand Up @@ -647,7 +646,7 @@ namespace unifex
template <typename...> class Variant,
template <typename...> class Tuple>
using value_types = typename sender_traits<SourceSender>::
template value_types<Variant, decayed_tuple<Tuple>::template apply>;
template value_types<Variant, Tuple>;

// This can produce any of the error_types of SourceSender, or of
// CompletionSender or an exception_ptr corresponding to an exception thrown
Expand Down
Loading