From bc736828f884c00fbc4d7d8147dfebc78d8a5843 Mon Sep 17 00:00:00 2001 From: Dmitry Kazakov Date: Thu, 22 Dec 2022 15:49:29 +0300 Subject: [PATCH] Fully fix the forwarding problems in getset lens The patch ensures the following requirements: 1) When `view(lens, whole)` is called: * the setter lambda is neither called nor constructed. It avoid unnecessary copies of the 'whole' object. * the `whole` object is directly forwarded into the getter lambda, without any copies. 3) When `set(lens, whole, part)` is called: * no getter is called at the toplevel lens (which value was previously discarded). * on the lower lenses, the getter receives an 'lvalue' reference to the `whole` object, and setter receives an 'rvalue' reference to the `whole` object. It ensures that there is no double-move problem. Fixes #160 --- lager/lenses.hpp | 131 +++++++++++--- test/lenses.cpp | 461 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 569 insertions(+), 23 deletions(-) diff --git a/lager/lenses.hpp b/lager/lenses.hpp index 2d7a970d..3be1c020 100644 --- a/lager/lenses.hpp +++ b/lager/lenses.hpp @@ -21,6 +21,15 @@ namespace lager { namespace detail { +template +struct should_move : + std::integral_constant && + !std::is_lvalue_reference_v> {}; + +template +constexpr bool should_move_v = should_move::value; + template struct const_functor; @@ -42,6 +51,15 @@ struct const_functor } }; +template +struct is_const_functor : public std::false_type {}; + +template +struct is_const_functor> : public std::true_type {}; + +template +constexpr bool is_const_functor_v = is_const_functor::value; + template struct identity_functor; @@ -59,8 +77,14 @@ struct identity_functor template auto operator()(Fn&& f) && { - return make_identity_functor( - std::forward(f)(std::forward(value))); + + if constexpr (!should_move_v) { + return make_identity_functor( + std::forward(f)(value)); + } else { + return make_identity_functor( + std::forward(f)(std::move(value))); + } } }; @@ -68,31 +92,96 @@ template struct identity_functor_skip_first { template - auto operator() (T&&) const { + auto operator() (T&&) const & { return make_identity_functor(part); } - Part ∂ + template + auto operator() (T&&) && { + if constexpr (!should_move_v) { + return make_identity_functor(part); + } else { + return make_identity_functor(std::move(part)); + } + } + + Part part; }; template -auto make_identity_functor_skip_first(T&& x) -> identity_functor_skip_first> +auto make_identity_functor_skip_first(T&& x) -> identity_functor_skip_first { return {std::forward(x)}; } -template -auto call_getter_or_skip(const Func& func, Getter&& getter, Whole&& whole) { - return func(LAGER_FWD(getter)(LAGER_FWD(whole))); -} +template +struct getset_t +{ + template + auto operator() (Whole &&w) { + if constexpr (is_const_functor_v) { + /** + * We don't have a setter here, so it is safe to + * jus pass `w` as an rvalue. + * + * We also know that we are calling the const_functor, + * and it discards the passed argument, so just pass a + * noop to it. + * + * This branch is taken when viewing through the lens. + */ + return f(getter(LAGER_FWD(w))) // pass `w` into the getter as an rvalue! + (zug::noop); + } else { + /** + * Here we have both, getter and setter, so we pass + * `w` to getter as an lvalue, and to setter as an rvalue. + * The setter has a chance to reuse the resources of + * the passed value. + * + * This branch is taken on all the levels of setting the + * value through except of the tompost level. + */ + return f(getter(w)) // pass `w` into the getter as an lvalue! + ([&](auto&& x) { + return setter(LAGER_FWD(w), LAGER_FWD(x)); + }); + } + } + + F f; + Getter getter; + Setter setter; +}; + +/** + * This specialization is called when a set() method is called over + * the lens. In such a case we can skip calling the getter branch + * of the lens. + * + * This branch is taken on the topmost level of setting the value + * through the lens. + */ +template +struct getset_t, Getter, Setter> +{ + template + auto operator() (Part &&p) { + return std::move(f)(zug::noop) + ([&](auto&& x) mutable { + return setter(LAGER_FWD(p), LAGER_FWD(x)); + }); + } + + identity_functor_skip_first &&f; + Getter getter; + Setter setter; +}; -template -auto call_getter_or_skip(const identity_functor_skip_first& func, Getter&&, Whole&&) { - /** - * When set() call is being executed, we should not execute the setter, - * the value will be dropped later anyway - */ - return func(*static_cast(nullptr)); +template +auto make_getset_t(F &&f, const Getter &getter, const Setter &setter) +{ + return getset_t{std::forward(f), getter, setter}; } } // namespace detail @@ -112,7 +201,7 @@ decltype(auto) view(LensT&& lens, T&& x) template decltype(auto) set(LensT&& lens, T&& x, U&& v) { - return lens(detail::make_identity_functor_skip_first(v)) ( + return lens(detail::make_identity_functor_skip_first(std::forward(v))) ( std::forward(x)) .value; } @@ -135,14 +224,10 @@ namespace lenses { //! @{ template -auto getset(Getter&& getter, Setter&& setter) +auto getset(const Getter& getter, const Setter& setter) { return zug::comp([=](auto&& f) { - return [&, f = LAGER_FWD(f)](auto&& p) { - return detail::call_getter_or_skip(f, getter, LAGER_FWD(p))([&](auto&& x) { - return setter(LAGER_FWD(p), LAGER_FWD(x)); - }); - }; + return detail::make_getset_t(LAGER_FWD(f), getter, setter); }); } diff --git a/test/lenses.cpp b/test/lenses.cpp index ada41b88..6adf69b7 100644 --- a/test/lenses.cpp +++ b/test/lenses.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include @@ -30,6 +31,14 @@ struct yearday { int day; int month; + + friend bool operator==(const yearday &lhs, const yearday &rhs) { + return lhs.day == rhs.day && lhs.month == rhs.month; + } + + friend bool operator!=(const yearday &lhs, const yearday &rhs) { + return !(lhs == rhs); + } }; struct person @@ -230,6 +239,458 @@ TEST_CASE("lenses, no getter on set") CHECK(numGetCalls == 2); } +TEST_CASE("lenses, no getter on nested set") +{ + int numGetCallsOnBirthDay = 0; + int numGetCallsOnMonth = 0; + auto birthday = attr3(&person::birthday, &numGetCallsOnBirthDay); + auto month = attr3(&yearday::month, &numGetCallsOnMonth); + auto p1 = person{{5, 4}, "juanpe"}; + + CHECK(view(birthday, p1) == yearday{5, 4}); + CHECK(numGetCallsOnBirthDay == 1); + CHECK(numGetCallsOnMonth == 0); + numGetCallsOnBirthDay = 0; + numGetCallsOnMonth = 0; + + CHECK(view(birthday | month, p1) == 4); + CHECK(numGetCallsOnBirthDay == 1); + CHECK(numGetCallsOnMonth == 1); + numGetCallsOnBirthDay = 0; + numGetCallsOnMonth = 0; + + p1 = set(birthday | month, p1, 6); + CHECK(numGetCallsOnBirthDay == 1); + CHECK(numGetCallsOnMonth == 0); + numGetCallsOnBirthDay = 0; + numGetCallsOnMonth = 0; + + CHECK(view(birthday | month, p1) == 6); + CHECK(numGetCallsOnBirthDay == 1); + CHECK(numGetCallsOnMonth == 1); + numGetCallsOnBirthDay = 0; + numGetCallsOnMonth = 0; +} + + +struct copy_info +{ + copy_info(std::string _debugName) : debug_name(std::move(_debugName)) {}; + + bool operator== (const std::tuple &rhs) const { + return num_copy_constructions == std::get<0>(rhs) && + num_copy_assignments == std::get<1>(rhs) && + num_move_constructions == std::get<2>(rhs) && + num_move_assignments == std::get<3>(rhs); + } + + bool operator!= (const std::tuple &rhs) const { + return !(*this == rhs); + } + + void reset() { + num_copy_constructions = 0; + num_copy_assignments = 0; + num_move_constructions = 0; + num_move_assignments = 0; + } + + int num_copy_constructions = 0; + int num_copy_assignments = 0; + int num_move_constructions = 0; + int num_move_assignments = 0; + std::string debug_name; +}; + +std::ostream& operator << (std::ostream &out, const copy_info &info) +{ + out << "copy_info("; + out << "cc:" << info.num_copy_constructions << " "; + out << "ca:" << info.num_copy_assignments << " "; + out << "mc:" << info.num_move_constructions << " "; + out << "ma:" << info.num_move_assignments; + out << ")"; + return out; +} + +struct copy_tracker +{ + copy_tracker(copy_info &info) + : info(info) + { + } + + copy_tracker(const copy_tracker &rhs) + : info(rhs.info) + { + assert(!rhs.moved_from_here); + info.num_copy_constructions++; + } + + copy_tracker(copy_tracker &&rhs) + : info(rhs.info) + { + assert(!rhs.moved_from_here); + rhs.moved_from_here = true; + info.num_move_constructions++; + } + + copy_tracker& operator=(const copy_tracker &rhs) + { + assert(!rhs.moved_from_here); + info = rhs.info; + info.num_copy_assignments++; + return *this; + } + + copy_tracker& operator=(copy_tracker &&rhs) + { + assert(!rhs.moved_from_here); + info = rhs.info; + rhs.moved_from_here = true; + info.num_move_assignments++; + return *this; + } + + copy_info &info; + bool moved_from_here = false; +}; + +struct debug_yearday +{ + debug_yearday(copy_info &info) + : tracker(info) + { + } + + bool valid() { + return !tracker.moved_from_here; + } + + copy_tracker tracker; + int day {0}; + int month {0}; +}; + +struct debug_person +{ + debug_person(copy_info &person_copy_info, copy_info &birthday_copy_info) + : tracker(person_copy_info) + , birthday(birthday_copy_info) + + { + } + + bool valid() { + return !tracker.moved_from_here && + !birthday.tracker.moved_from_here; + } + + copy_tracker tracker; + debug_yearday birthday; + std::string name; + std::vector things{}; +}; + +struct debug_freelancer +{ + debug_freelancer(copy_info &freelancer_copy_info, copy_info &person_copy_info, copy_info &birthday_copy_info) + : tracker(freelancer_copy_info) + , person(person_copy_info, birthday_copy_info) + { + } + + bool valid() { + return !tracker.moved_from_here && + !person.tracker.moved_from_here && + !person.birthday.tracker.moved_from_here; + } + + copy_tracker tracker; + debug_person person; + int tax_id {}; +}; + +TEST_CASE("getset copy: view lvalue") +{ + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + auto birthday_lens = attr2(&debug_person::birthday); + + auto p1 = debug_person(person_copy_info, birthday_copy_info); + + CHECK(person_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(birthday_copy_info == std::make_tuple(0, 0, 0, 0)); + + auto birthday = view(birthday_lens, p1); + + CHECK(person_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(birthday_copy_info == std::make_tuple(1, 0, 0, 0)); + + CHECK(p1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("getset copy: view rvalue") +{ + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + + auto birthday = view(birthday_lens, debug_person(person_copy_info, birthday_copy_info)); + + CHECK(person_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(birthday_copy_info == std::make_tuple(0, 0, 3, 0)); + + CHECK(birthday.valid()); +} + + +TEST_CASE("getset copy: set lvalue with rvalue") +{ + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + + auto p1 = debug_person(person_copy_info, birthday_copy_info); + + auto birthday = + set(birthday_lens, p1, debug_yearday(birthday_copy_info)); + + CHECK(person_copy_info == std::make_tuple(1, 0, 3, 0)); + CHECK(birthday_copy_info == std::make_tuple(1, 0, 5, 1)); + + CHECK(p1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("getset copy: set lvalue with lvalue") +{ + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + + auto p1 = debug_person(person_copy_info, birthday_copy_info); + auto day1 = debug_yearday(birthday_copy_info); + + auto birthday = set(birthday_lens, p1, day1); + + CHECK(person_copy_info == std::make_tuple(1, 0, 3, 0)); + CHECK(birthday_copy_info == std::make_tuple(1, 1, 3, 0)); + + CHECK(p1.valid()); + CHECK(day1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("getset copy: set rvalue with lvalue") +{ + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + + auto day1 = debug_yearday(birthday_copy_info); + + auto birthday = + set(birthday_lens, debug_person(person_copy_info, birthday_copy_info), day1); + + CHECK(person_copy_info == std::make_tuple(0, 0, 4, 0)); + CHECK(birthday_copy_info == std::make_tuple(0, 1, 4, 0)); + + CHECK(day1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("getset copy: set rvalue with rvalue") +{ + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + + auto birthday = + set(birthday_lens, + debug_person(person_copy_info, birthday_copy_info), + debug_yearday(birthday_copy_info)); + + CHECK(person_copy_info == std::make_tuple(0, 0, 4, 0)); + CHECK(birthday_copy_info == std::make_tuple(0, 0, 6, 1)); + + CHECK(birthday.valid()); +} + +TEST_CASE("nested getset copy: view lvalue") +{ + copy_info freelancer_copy_info("freelancer"); + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + auto birthday_lens = attr2(&debug_person::birthday); + auto person_lens = attr2(&debug_freelancer::person); + auto freelancer_birthday_lens = person_lens | birthday_lens; + + auto e1 = debug_freelancer(freelancer_copy_info, person_copy_info, birthday_copy_info); + + CHECK(person_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(birthday_copy_info == std::make_tuple(0, 0, 0, 0)); + + auto birthday = view(freelancer_birthday_lens, e1); + + CHECK(freelancer_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(person_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(birthday_copy_info == std::make_tuple(1, 0, 0, 0)); + + CHECK(e1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("nested getset copy: view rvalue") +{ + copy_info freelancer_copy_info("freelancer"); + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + auto birthday_lens = attr2(&debug_person::birthday); + auto person_lens = attr2(&debug_freelancer::person); + auto freelancer_birthday_lens = person_lens | birthday_lens; + + CHECK(person_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(birthday_copy_info == std::make_tuple(0, 0, 0, 0)); + + auto birthday = + view(freelancer_birthday_lens, + debug_freelancer(freelancer_copy_info, person_copy_info, birthday_copy_info)); + + CHECK(freelancer_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(person_copy_info == std::make_tuple(0, 0, 0, 0)); + CHECK(birthday_copy_info == std::make_tuple(0, 0, 4, 0)); + + CHECK(birthday.valid()); +} + +TEST_CASE("nested getset copy: set lvalue with rvalue") +{ + copy_info freelancer_copy_info("freelancer"); + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + auto person_lens = attr2(&debug_freelancer::person); + auto freelancer_birthday_lens = person_lens | birthday_lens; + + auto e1 = debug_freelancer(freelancer_copy_info, person_copy_info, birthday_copy_info); + + auto birthday = + set(freelancer_birthday_lens, + e1, + debug_yearday(birthday_copy_info)); + + CHECK(freelancer_copy_info == std::make_tuple(1, 0, 3, 0)); + + /** + * One copy is for passing a person into the setter of birthday_lens, + * the other one is for passing the source enterpreneur into the + * setter function of person_lens. + */ + CHECK(person_copy_info == std::make_tuple(2, 0, 5, 1)); + CHECK(birthday_copy_info == std::make_tuple(2, 0, 7, 2)); + + CHECK(e1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("nested getset copy: set lvalue with lvalue") +{ + copy_info freelancer_copy_info("freelancer"); + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + auto person_lens = attr2(&debug_freelancer::person); + auto freelancer_birthday_lens = person_lens | birthday_lens; + + auto e1 = debug_freelancer(freelancer_copy_info, person_copy_info, birthday_copy_info); + auto day1 = debug_yearday(birthday_copy_info); + + auto birthday = + set(freelancer_birthday_lens, + e1, + day1); + + CHECK(freelancer_copy_info == std::make_tuple(1, 0, 3, 0)); + + /** + * One copy is for passing a person into the setter of birthday_lens, + * the other one is for passing the source enterpreneur into the + * setter function of person_lens. + */ + CHECK(person_copy_info == std::make_tuple(2, 0, 5, 1)); + CHECK(birthday_copy_info == std::make_tuple(2, 1, 5, 1)); + + CHECK(e1.valid()); + CHECK(day1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("nested getset copy: set rvalue with lvalue") +{ + copy_info freelancer_copy_info("freelancer"); + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + auto person_lens = attr2(&debug_freelancer::person); + auto freelancer_birthday_lens = person_lens | birthday_lens; + + auto day1 = debug_yearday(birthday_copy_info); + + auto birthday = + set(freelancer_birthday_lens, + debug_freelancer(freelancer_copy_info, person_copy_info, birthday_copy_info), + day1); + + CHECK(freelancer_copy_info == std::make_tuple(0, 0, 4, 0)); + + /** + * The only copy is to pass the person into birthday_lens. Use const-ref + * for `whole` argument to avoid this copy. + */ + CHECK(person_copy_info == std::make_tuple(1, 0, 6, 1)); + CHECK(birthday_copy_info == std::make_tuple(1, 1, 6, 1)); + + CHECK(day1.valid()); + CHECK(birthday.valid()); +} + +TEST_CASE("nested getset copy: set rvalue with rvalue") +{ + copy_info freelancer_copy_info("freelancer"); + copy_info person_copy_info("person"); + copy_info birthday_copy_info("birthday"); + + auto birthday_lens = attr2(&debug_person::birthday); + auto person_lens = attr2(&debug_freelancer::person); + auto freelancer_birthday_lens = person_lens | birthday_lens; + + auto birthday = + set(freelancer_birthday_lens, + debug_freelancer(freelancer_copy_info, person_copy_info, birthday_copy_info), + debug_yearday(birthday_copy_info)); + + CHECK(freelancer_copy_info == std::make_tuple(0, 0, 4, 0)); + + /** + * The only copy is to pass the person into birthday_lens. Use const-ref + * for `whole` argument to avoid this copy. + */ + CHECK(person_copy_info == std::make_tuple(1, 0, 6, 1)); + CHECK(birthday_copy_info == std::make_tuple(1, 0, 8, 2)); + + CHECK(birthday.valid()); +} + TEST_CASE("lenses, at immutable index") { auto first = at(0);