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 1 commit
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
18 changes: 9 additions & 9 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,7 @@
namespace unifex {
namespace _create {

template <typename Receiver, typename Fn, typename Context>
template <typename Receiver, typename Fn, typename Context, typename... ValueTypes>
struct _op {
struct type {
explicit type(Receiver rec, Fn fn, Context ctx)
Expand All @@ -36,7 +36,7 @@ struct _op {
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...);
unifex::set_value((Receiver&&) rec_, (ValueTypes) ts...);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a few thoughts.

First, I think there's a chance that the conversion from Ts&& to ValueTypes might throw, right? If so, the current noexcept declaration is incomplete.

Second, I intend to change set_value be noexcept (it simplifies many things, saves binary size, and brings us into better alignment with P2300) so I think, rather than refining the noexcept clause, it'd probably be better to just make it noexcept and forward exceptions to set_error.

Finally, we've been standardizing on an unstated coding style: use std::move() for moves and static_cast<Dest>(value) to replace std::forward, and don't use C-style casts. As I understand it, the C-style casts are in the code to cut down on compile time by avoiding template instantiations. We've written a handful of bugs by following this style, though, so I'm not convinced the build-time speedups are worth the maintenance risk; we may yet migrate from static_cast to std::forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First, I think there's a chance that the conversion from Ts&& to ValueTypes might throw, right? If so, the current noexcept declaration is incomplete.

Thanks, good catch.

Second, I intend to change set_value be noexcept

Done. Makes sense. Once set_value is mandated to be noexcept, we can do the same cosntexpr conditional to avoid the try/catch if the conversion is nonthrowing, but I have not made that change here.

Finally, we've been standardizing on an unstated coding style: use std::move() for moves and static_cast(value) to replace std::forward, and don't use C-style casts.

I made one step towards this within this PR, although I think I have at least one more place to update within my changes. Thanks for clarifying the standard within this project. I will note, clang-tidy has been adding CppCoreGuidelines checks for correct move/forward/universal parameter usage (e.g., F.18) and I think the checks work best (or in some cases, only work) when move and forward are used rather than casts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will note, clang-tidy [fixes this]

Huh. We should take advantage. I've filed issue #548.

Copy link
Contributor

Choose a reason for hiding this comment

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

The conversion from Ts&& to ValueTypes has me a bit squirrely. The whole point of this change is to preserve the cvref qualifiers on the type parameters to create<>() so we obviously need to perform this conversion anytime set_value is invoked with inexactly-typed parameters, but I'm not sure I fully understand all the corner cases.

I think it's right for the function to take arbitrary forwarding references (rather than, say, ValueTypes&&...) so that:

  • natural implicit conversions can happen (e.g. a create<double> that is completed with set_value(0) ought to work), and
  • any exceptions that arise happen inside set_value so that we can capture-and-forward them automatically.

But, I worry that we'll copy or move more often than necessary if we get the details wrong.

Maybe the solution is to factor the conversion into a functor that performs the operation that convertible_to checks for?

// defined at namespace scope
template <typename From, typename To, typename = void>
struct do_convert {
  // default to no conversion
  To operator()(From&&) = delete;
};

template <typename From>
struct do_convert<From, From, void> {
  // don't construct anything when "conversion" is the identity
  From&& operator()(From&& obj) noexcept {
    return static_cast<From&&>(obj);
  }
};

template <typename From, typename To>
struct do_convert<From, To, std::enable_if_t<convertible_to<From, To>>> {
  // explicitly perform the operation that we've checked is implicitly allowed
  To operator()(From&& obj) {
    return static_cast<From&&>(obj);
  }
};

template(typename T...) //
    (requires (convertible_to<T, ValueTypes> && ...)) //
void set_value(T&&... values) noexcept {
  UNIFEX_TRY {
    unifex::set_value(std::move(rec_), do_convert<T, ValueTypes>{}(static_cast<T&&>(values))...);
  }
  UNIFEX_CATCH(...) {
    unifex::set_error(std::move(rec_), std::current_exception());
  }
}

The part of the above I'm most uncertain about is the type parameters given to do_convert. Should it be

  • do_convert<T, ValueTypes>,
  • do_convert<T&&, ValueTypes>,
  • do_convert<T, ValueTypes&&>, or
  • do_convert<T&&, ValueTypes&&>?

For create<cv int&> and set_value invoked with a cv int&, they're all equivalent because of reference collapsing and they'll all pick the identity, which avoids unnecessary copies/moves and correctly forwards "the same" int from the set_value call site through to the Receiver's set_value.

For create<cv int&> and set_value invoked with an rvalue int or with anything convertible to an int, like a cvref-qualified double, all of the above options will find the default template expansion and (correctly) block the conversion.

How do create<cv int> and create<cv int&&> differ? There is a difference between

std::unique_ptr<int> foo() {
  return std::move(*this).foo_;
}

and

std::unique_ptr<int>&& foo() {
  return std::move(*this).foo_;
}

For one thing foo_ is unconditionally left null in the first one, but the second one is conditional, depending on what the caller does with the result.

Perhaps the second type argument to do_convert should be ValueTypes and not ValueTypes&& because it would allow us to distinguish between create<int> and create<int&&>.

To get the unconditional move behaviour for create<int> and the conditional move behaviour for create<int&&>, I think we need the first type argument to do_convert to be T&&; that will choose the identity "conversion" (and thus maintain reference stability) when ValueTypes is an rvalue reference, and pick the T&& -> T conversion when ValueTypes is not a reference at all.

So that leaves us with

unifex::set_value(std::move(rec_), do_convert<T&&, ValueTypes>{}(static_cast<T&&>(values))...);

Does my reasoning seem sound?

If so, is do_convert even necessary? Maybe I've just complicated a cast expression.

Should this be

Suggested change
unifex::set_value((Receiver&&) rec_, (ValueTypes) ts...);
UNIFEX_TRY {
unifex::set_value(std::move(rec_), ValueTypes{static_cast<Ts&&>(ts)}...);
}
UNIFEX_CATCH(...) {
unifex::set_error(std::move(rec_), std::current_exception());
}

with a requires (convertible_to<Ts, ValueTypes> && ...) clause on the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, I worry that we'll copy or move more often than necessary if we get the details wrong.

I had a similar concern. My original logic of set_value((Receiver&&) rec_, (ValueTypes) ts...) definitely incurs unnecessary copies or moves (I've added tests in my latest commit to capture this number of copies or moves to ensure no unnecessary copies or moves are done).

I'm having a hard time understanding whether do_convert<Ts&&, ValueTypes> or do_convert<Ts, ValueTypes> is right. Consider calling set_value<AnObject> on create<AnObject>. I think this will not hit the do_convert<From, From, void> specialization if do_convert<Ts&&, ValueTypes> is used and instead will end up calling the other which leads to a copy of the Object. In the latest commit I pushed, my new tests pass with both versions, so I don't think I have a test that exposes the right condition where Ts&& vs Ts in do_convert matters. I did add a test for the "conditional move" behavior (create<TrackingObject&&>).

Ts ValueTypes Desired Result passed to the inner set_value do_convert<Ts, ValueTypes> do_convert<Ts&&, ValueTypes>
int int int&& int&& int
int& int int int int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, I added Co-authored-by: Ian Petersen <ispeters@fb.com> in the commit where I used do_convert. LMK if you are OK with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having a hard time understanding whether do_convert<Ts&&, ValueTypes> or do_convert<Ts, ValueTypes> is right. Consider calling set_value<AnObject> on create<AnObject>. I think this will not hit the do_convert<From, From, void> specialization if do_convert<Ts&&, ValueTypes> is used and instead will end up calling the other which leads to a copy of the Object.

I'm currently leaning towards "create<AnObject> ought to always construct a new object" (whether by copy or by move is up to the value category of the argument to set_value). In other words, do_convert<Ts&&, ValuteTypes> not matching do_convert<From, From, void> is, I think, the right choice. If there are scenarios where create<AnObject> doesn't create a new object then I think it's indistinguishable from create<AnObject&&>, right?

If I think of set_value as "how Sender/Receiver implements return" then requiring create<AnObject> to always create a new object is akin to saying "Sender/Receiver doesn't have RVO," which I think is just true.

I think there are two choices here with different trade-offs:

  1. create<AnObject> always creates a new object and is distinguishable from create<AnObject&&>, or
  2. create<AnObject> is the same as create<AnObject&&> and the algorithm never constructs a new object for you except when performing an implicit conversion.

I think the danger of 1 is unintentional copy/move, which can be "fixed" by changing to create<AnObject&&> and the danger of 2 is unintentional aliasing, which can be "fixed" by deliberately passing a temporary to set_value (i.e. set_value(AnObject{anObject})) or by receiving the value(s) by value (e.g. | then([](auto a) { return a; })).

I feel like the danger of 1 is less dangerous than the danger of 2 and the cost of safety isn't that high, but what do you think?

Also, if we make create<AnObject> and create<AnObject&&> the same, have we thrown away the ability to send values in the pursuit of the ability to send references?

Copy link
Contributor

@ispeters ispeters Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Re: giving me co-authorship credit, that's kind of you. I feel like I'm playing the role of editor here, not co-author, so it feels a bit like undue credit, but I don't know what the open source conventions are around this kind of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now I see - I think create<AnObject> should construct an object, that seems like the right semantics for "how Sender/Receiver implement return." Thanks for explaining the key difference. I added a test CreateObjectLeadsToNewObject which only works with do_convert<Ts&&, ValueTypes>, which ensures that the move constructor is invoked at least once (it's never invoked with do_convert<Ts, ValueTypes>).

Re: credit - im not sure of the convention either. Not a big deal, and fair point about being "editor," so I rebased and removed it.

}

template (typename Error)
Expand Down Expand Up @@ -79,10 +79,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 +101,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 +121,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
51 changes: 41 additions & 10 deletions test/create_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
using namespace unifex;

namespace {

int global;

struct CreateTest : testing::Test {
unifex::single_thread_context someThread;
unifex::async_scope someScope;
Expand All @@ -47,6 +50,14 @@ struct CreateTest : testing::Test {
});
}

void anIntRefAPI(void* context, void (*completed)(void* context, int& result)) {
// Execute some work asynchronously on some other thread. When its
// work is finished, pass the result to the callback.
someScope.detached_spawn_call_on(someThread.get_scheduler(), [=]() noexcept {
completed(context, global);
});
}

void aVoidAPI(void* context, void (*completed)(void* context)) {
// Execute some work asynchronously on some other thread. When its
// work is finished, pass the result to the callback.
Expand All @@ -58,18 +69,38 @@ struct CreateTest : testing::Test {
} // anonymous namespace

TEST_F(CreateTest, BasicTest) {
auto snd = [this](int a, int b) {
return create<int>([a, b, this](auto& rec) {
static_assert(receiver_of<decltype(rec), int>);
anIntAPI(a, b, &rec, [](void* context, int result) {
unifex::void_cast<decltype(rec)>(context).set_value(result);
{
auto snd = [this](int a, int b) {
return create<int>([a, b, this](auto& rec) {
static_assert(receiver_of<decltype(rec), int>);
anIntAPI(a, b, &rec, [](void* context, int result) {
unifex::void_cast<decltype(rec)>(context).set_value(result);
});
});
});
}(1, 2);
}(1, 2);

std::optional<int> res = sync_wait(std::move(snd));
ASSERT_TRUE(res.has_value());
EXPECT_EQ(*res, 3);
std::optional<int> res = sync_wait(std::move(snd));
ASSERT_TRUE(res.has_value());
EXPECT_EQ(*res, 3);
}

{
auto snd = [this]() {
return create<int&>([this](auto& rec) {
static_assert(receiver_of<decltype(rec), int&>);
anIntRefAPI(&rec, [](void* context, int& result) {
unifex::void_cast<decltype(rec)>(context).set_value(result);
});
});
}();

std::optional<std::reference_wrapper<int>> res = sync_wait(std::move(snd));
ASSERT_TRUE(res.has_value());
global = 0;
EXPECT_EQ(*res, 0);
global = 10;
EXPECT_EQ(*res, 10);
ccotter marked this conversation as resolved.
Show resolved Hide resolved
}
}

TEST_F(CreateTest, VoidWithContextTest) {
Expand Down
116 changes: 112 additions & 4 deletions test/finally_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,24 @@
* limitations under the License.
*/
#include <unifex/finally.hpp>

#include <unifex/inplace_stop_token.hpp>
#include <unifex/sync_wait.hpp>
#include <unifex/timed_single_thread_context.hpp>
#include <unifex/scheduler_concepts.hpp>
#include <unifex/then.hpp>
#include <unifex/just.hpp>
#include <unifex/just_done.hpp>
#include <unifex/just_error.hpp>
#include <unifex/just_from.hpp>
#include <unifex/let_done.hpp>
#include <unifex/let_error.hpp>
#include <unifex/scheduler_concepts.hpp>
#include <unifex/sync_wait.hpp>
#include <unifex/then.hpp>
#include <unifex/timed_single_thread_context.hpp>
#include <unifex/upon_error.hpp>

#include <cstdio>
#include <thread>
#include <tuple>
#include <variant>

#include <gtest/gtest.h>

Expand All @@ -45,6 +50,109 @@ TEST(Finally, Value) {
EXPECT_EQ(res->second, context.get_thread_id());
}

TEST(Finally, Ref) {
{
int a = 0;

auto sndr = just_from([&a]() -> int& { return a; })
| finally(just());
using Sndr = decltype(sndr);

static_assert(std::is_same_v<
Sndr::value_types<std::variant, std::tuple>,
std::variant<std::tuple<int&>>
>);
static_assert(std::is_same_v<
Sndr::error_types<std::variant>,
std::variant<std::exception_ptr>
>);
static_assert(!Sndr::sends_done);
ccotter marked this conversation as resolved.
Show resolved Hide resolved

auto res = std::move(sndr) | sync_wait();

ASSERT_FALSE(!res);
EXPECT_EQ(*res, 0);
a = 10;
EXPECT_EQ(*res, 10);
}

{
int a = 0;

auto res = just_from([&a]() -> const int& { return a; })
| finally(just())
| sync_wait();

ASSERT_FALSE(!res);
EXPECT_EQ(*res, 0);
a = 10;
EXPECT_EQ(*res, 10);
}

{
int a = 0;

auto res = just_from([&a]() -> int& { return a; })
| finally(just())
| then([](int& i) -> int& { return i; })
| sync_wait();

ASSERT_FALSE(!res);
EXPECT_EQ(*res, 0);
a = 10;
EXPECT_EQ(*res, 10);
}
}

struct sends_error_ref {

template <
template <typename...> class Variant,
template <typename...> class Tuple>
using value_types = Variant<Tuple<>>;

template <template <typename...> class Variant>
using error_types = Variant<int>;

static constexpr bool sends_done = false;

template <class Receiver>
struct operation {
friend auto tag_invoke(tag_t<start>, operation& self) noexcept {
set_error(std::move(self.receiver), self.val);
}

int& val;
Receiver receiver;
};

template <class Receiver>
friend auto tag_invoke(tag_t<connect>, sends_error_ref self, Receiver&& receiver) {
return operation<Receiver>{self.val, std::forward<Receiver>(receiver)};
}

int& val;
};


TEST(Finally, ErrorRefDecays) {
// TODO: Should errors also have references preserved in 'finally?' See the
// 'finally' discussion in issue #541.
int a = 0;
auto sndr = sends_error_ref{a} | finally(just());
using Sndr = decltype(sndr);

static_assert(std::is_same_v<
Sndr::value_types<std::variant, std::tuple>,
std::variant<std::tuple<>>
>);
static_assert(std::is_same_v<
Sndr::error_types<std::variant>,
std::variant<int, std::exception_ptr>
>);
static_assert(!Sndr::sends_done);
ccotter marked this conversation as resolved.
Show resolved Hide resolved
}

TEST(Finally, Done) {
timed_single_thread_context context;

Expand Down