Skip to content

Commit

Permalink
Fix extra call to 'getter' in 'lenses::getset' on set call
Browse files Browse the repository at this point in the history
Basically, we shouldn't calculate the getter when we know that the
value will be dropped.

The patch partially fixes arximboldi#160, because now the getter is not called
at all during the set operation. But the double-move can still
happen when both, getter and setter calls are performed.
  • Loading branch information
dimula73 committed Nov 21, 2022
1 parent 24887ac commit b863b87
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 4 deletions.
38 changes: 34 additions & 4 deletions lager/lenses.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,37 @@ struct identity_functor
std::forward<Fn>(f)(std::forward<T>(value)));
}
};

template <typename Part>
struct identity_functor_skip_first
{
auto operator() (auto&&) const {
return make_identity_functor(part);
}

Part &part;
};

template <typename T>
auto make_identity_functor_skip_first(T&& x) -> identity_functor_skip_first<std::remove_reference_t<T>>
{
return {std::forward<T>(x)};
}

template <typename Func, typename Getter, typename Whole>
auto call_getter_or_skip(const Func& func, Getter&& getter, Whole&& whole) {
return func(LAGER_FWD(getter)(LAGER_FWD(whole)));
}

template <typename Getter, typename Part, typename Whole>
auto call_getter_or_skip(const identity_functor_skip_first<Part>& 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<Part*>(nullptr));
}

} // namespace detail

//! @defgroup lenses-api
Expand All @@ -80,7 +111,7 @@ decltype(auto) view(LensT&& lens, T&& x)
template <typename LensT, typename T, typename U>
decltype(auto) set(LensT&& lens, T&& x, U&& v)
{
return lens([&v](auto&&) { return detail::make_identity_functor(v); })(
return lens(detail::make_identity_functor_skip_first(v)) (
std::forward<T>(x))
.value;
}
Expand All @@ -107,9 +138,8 @@ auto getset(Getter&& getter, Setter&& setter)
{
return zug::comp([=](auto&& f) {
return [&, f = LAGER_FWD(f)](auto&& p) {
return f(getter(std::forward<decltype(p)>(p)))([&](auto&& x) {
return setter(std::forward<decltype(p)>(p),
std::forward<decltype(x)>(x));
return detail::call_getter_or_skip(f, getter, LAGER_FWD(p))([&](auto&& x) {
return setter(LAGER_FWD(p), LAGER_FWD(x));
});
};
});
Expand Down
30 changes: 30 additions & 0 deletions test/lenses.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ using namespace lager;
using namespace lager::lenses;
using namespace zug;


TEST_CASE("lenses, minimal example")
{
auto month = zug::comp([](auto&& f) {
Expand Down Expand Up @@ -200,6 +201,35 @@ TEST_CASE("lenses, attr2, references")
}
}

template <typename Member>
auto attr3(Member member, int *numGetCalls)
{
return getset(
[=](auto&& x) -> decltype(auto) {
(*numGetCalls)++;
return std::forward<decltype(x)>(x).*member;
},
[=](auto x, auto&& v) {
x.*member = std::forward<decltype(v)>(v);
return x;
});
};

TEST_CASE("lenses, no getter on set")
{
int numGetCalls = 0;
auto name = attr3(&person::name, &numGetCalls);
auto p1 = person{{5, 4}, "juanpe"};
CHECK(view(name, p1) == "juanpe");
CHECK(numGetCalls == 1);

p1 = set(name, p1, "ncopernicus");
CHECK(numGetCalls == 1);

CHECK(view(name, p1) == "ncopernicus");
CHECK(numGetCalls == 2);
}

TEST_CASE("lenses, at immutable index")
{
auto first = at(0);
Expand Down

0 comments on commit b863b87

Please sign in to comment.