Skip to content
This repository was archived by the owner on Jun 1, 2022. It is now read-only.
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions sim/lib/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ add_library(core-test
src/ics/util/composable.test.cpp
src/ics/util/setIntersection.test.cpp
src/ics/util/typeMap.test.cpp
src/optics/lens.test.cpp
)
target_link_libraries(core-test
PRIVATE core
Expand Down
179 changes: 179 additions & 0 deletions sim/lib/core/include/core/optics/lens.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
#ifndef BENTOBOX_LENS_HPP
#define BENTOBOX_LENS_HPP

#include <functional>
#include <type_traits>

namespace {
template <class... Args>
struct is_same_args {
// This static constexpr function compares the given type arguments and
// ensures that they are the same as Args
template <size_t N = 0, class... OtherArgs>
static constexpr bool _as() {
// constexpr is required here because N is a non-type template
// argument
if constexpr (N == sizeof...(OtherArgs)) {
return true;
} else if (std::is_same_v<
std::remove_reference_t<
std::tuple_element_t<N, std::tuple<OtherArgs...>>>,
std::remove_reference_t<
std::tuple_element_t<N, std::tuple<Args...>>>>) {
return _as<N + 1, OtherArgs...>();
} else {
return false;
}
}

// Alias to the recursive comparison constexpr function
template <class... OtherArgs>
static constexpr bool as() {
return _as<0, OtherArgs...>();
}
};
Comment on lines +12 to +34

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Possible to convert this into a C++ 17 fold expression?

} // namespace

namespace optics {

// Note on std::move vs static_cast<std::remove_reference_t<T>&&>
// Later in the code, you will see that I have used static_cast instead of
// std::move. The reason is that we need to cast to an rvalue reference
// because that's the type of the function's parameter.
// std::move is not used because it is semantically incorrect, though it
// does the same thing.

template <class Return, class... Args>
class Getter {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hard to understand Getter by name. From what i read in code it seems to be like a SQL SELECT statement where you can perform computations from selected values:

SELECT 
# equivalent to the lambda/function passed to Getter() constructor 
maxAge - age 
FROM 
# values passed via Getter.get()
getter_values

In that case, maybe selector/prism/transform might be a better name than Getter?

protected:
public:
const std::function<Return(std::remove_reference_t<Args>&&...)> getter;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Naming this function getter is too close to Getter, making the code hard to follow.

// A getter cannot be constructed without a getter function
Getter() = delete;
explicit Getter(std::function<Return(Args...)>&& getter)
: getter(std::move(getter)) {}
explicit Getter(const std::function<Return(Args...)>& getter)
: getter(getter) {}

template <class... RArgs>
requires(is_same_args<Args...>::template as<RArgs...>()) Return

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Assuming that I understand is_same_args<>::as<>() correctly, this defines a requirement that RArgs contains the exact same set of types as Args.
In that case, why can't we just use Args instead of having both RArgs & Args and then comparing if they are equal?

get(RArgs&&... args) const {
return getter(static_cast<std::remove_reference_t<RArgs>>(args)...);
}

template <class NewReturn>
Getter<NewReturn, Args...> compose(
std::function<NewReturn(Return)>&& composeGetter) const {
// Make a copy of the getter so that if the current Getter is destroyed
// The returned getter is still usable
Comment on lines +67 to +68

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In that case, why can't we have a pass-by-value copy constructor for Getter to be always assured a copy of the getter function for every Getter.

std::function<Return(Args...)> getterCopy(getter);
return Getter<NewReturn, Args...>(
[getterCopy, composeGetter](Args&&... args) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[getterCopy, composeGetter] passes by value by the way. Unsure if this is intended due to the explicit copy of getter above. Pass by reference should be [&getterCopy, &composeGetter].

return std::invoke(
composeGetter,
std::invoke(getterCopy, std::forward<Args>(args)...));
});
}

// Oddly, C++ does not recognise lambdas when std::function is used here.
// So, C++20 requirements are used.
// The attempted definition of a std::function is:
// template<class...NewArgs>
// Getter<Return, NewArgs...>
// precompose(std::function<std::tuple_element_t<0,
// std::tuple<Args...>>(NewArgs...)> precomposeGetter) {
template <class... NewArgs, class Fn>
// Require that the current getter only needs one argument
requires(sizeof...(Args) == 1) &&
// Require that the type Fn is something invocable with the NewArgs and
// returns the first argument
std::is_invocable_r_v<
std::tuple_element_t<0, std::tuple<Args...>>, Fn,
NewArgs...> Getter<Return,
NewArgs...> precompose(Fn&& precomposeGetter)
Comment on lines +85 to +93

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use typedef/using so that function declarations are easier to read.

const {
Comment on lines +85 to +94

@mrzzy mrzzy Feb 23, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Given functions A and B,
it appears that A.compose(B) does the same thing as B.precompose(A).
Is it completely necessary to have precompose() considering how complex it is?

// Make a copy of the getter so that if the current Getter is destroyed
// The returned getter is still usable
std::function<Return(Args...)> getterCopy(getter);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See comments above about getterCopy.

return Getter<Return, NewArgs...>(
[getterCopy, precomposeGetter](NewArgs&&... args) {
return std::invoke(getterCopy,
std::invoke(precomposeGetter,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is the benefit of using std::invoke instead of calling getterCopy() or precomposeGetter() directly?

std::forward<NewArgs>(args)...));
});
}
};

template <class Return, class... Args>
class Lens {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Seems like a Lens is a read/write version of a Getter, although it seems that they do not share a codebase.

  • why does is the readonly part of Lens not implemented in terms of Getter?
  • why do we need the setters in lens? why cant all value transforms be implemented in terms of a getter + = assignment and eliminate setters?.

Naming: Name wise they seem to completely unrelated.

Without being used by actual code by the Engine codebase, I don't understand we need a Lens.

private:
typedef std::function<void(Return&, Return&&)> SetterFn;
// When writing the getter as a lambda, it is important to specify the
// arguments as rvalue references and the return type as an lvalue
// reference.
typedef std::function<Return&(Args&&...)> GetterFn;
Comment on lines +110 to +114

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not sure what the point of making these private is.


public:
const SetterFn setter;
const GetterFn getter;
// A lens cannot be constructed without any functions
Lens() = delete;
Lens(GetterFn&& getter, SetterFn&& setter)
: getter(std::move(getter)), setter(std::move(setter)) {}
Lens(const GetterFn& getter, const SetterFn& setter)
: getter(getter), setter(setter) {}

template <class... RArgs>
requires(is_same_args<Args...>::template as<RArgs...>()) Return& get(
RArgs&&... args) const {
return getter(static_cast<std::remove_reference_t<RArgs>&&>(args)...);
}

template <class RReturn, class... RArgs>
// Ensure that the arguments are the correct type
requires(is_same_args<Args...>::template as<RArgs...>())
// Ensure that RReturn is the correct type
&& std::is_same_v<std::remove_reference_t<RReturn>, Return> void set(
RReturn&& val, RArgs&&... args) const {
auto& ref = get(std::forward<RArgs>(args)...);
this->setter(ref, static_cast<std::remove_reference_t<RReturn>&&>(val));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Consistency. Choose whether you would like to use this-> or not. Earlier getter was used without this->.

}

template <class NewReturn>
Lens<NewReturn, Args...> compose(
const Lens<NewReturn, Return>& otherLens) const {
std::function<NewReturn&(Args && ...)> get =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose another name. This one overshadows get() the method which might not be intended.

[
getter = this->getter, otherGetter = otherLens.getter
]<class... PArgs>(PArgs && ... args)
->NewReturn& {
return otherGetter(
static_cast<Return&&>(getter(std::forward<PArgs>(args)...)));
};

return Lens<NewReturn, Args...>(get, otherLens.setter);
}

template <class FirstArg, class... NewArgs>
requires(sizeof...(Args) == 1) &&
std::is_same_v<
std::tuple_element_t<0, std::tuple<Args...>>,
FirstArg> Lens<Return, NewArgs...> precompose(const Lens<FirstArg,
NewArgs...>
otherLens) const {
std::function<Return&(NewArgs && ...)> get =
[
getter = this->getter, otherGetter = otherLens.getter
]<class... PArgs>(PArgs && ... args)
->Return& {
return getter(static_cast<FirstArg&&>(
otherGetter(std::forward<PArgs>(args)...)));
};

return Lens<Return, NewArgs...>(get, this->setter);
}
};

} // namespace optics

#endif // BENTOBOX_LENS_HPP
135 changes: 135 additions & 0 deletions sim/lib/core/src/optics/lens.test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
#include <gtest/gtest.h>
#include <core/optics/lens.hpp>

#define TEST_SUITE Lenstest

using namespace optics;

TEST(TEST_SUITE, GetterFromLambda) {
// From rvalue
{
auto getter = Getter<char, int, int>([](int j, int k) { return 'a'; });
ASSERT_EQ(getter.get(5, 7), 'a');
}

// From lvalue
{
auto lambdaGetter = []<class T, class U>(T&& j, U&& k) {
return j + k;
};
auto getter = Getter<int, int, int>(lambdaGetter);
// This is to test that get can accept an lvalue
auto val = 9;
ASSERT_EQ(getter.get(val, 10), 19);
}
}

TEST(TEST_SUITE, ComposeGetters) {
auto getter = Getter<int, int, int>([](int j, int k) { return j + k; });
auto composedGetter = getter.compose<int>([](int h) { return h + 20; });

ASSERT_EQ(composedGetter.get(1, 2), 23);
}

TEST(TEST_SUITE, PrecomposeGetters) {
auto getter = Getter<int, int>([](int j) { return j * 10; });
auto precomposedGetter =
getter.precompose<int>([](int h) -> int { return h + 20; });

ASSERT_EQ(precomposedGetter.get(1), 210);
}

TEST(TEST_SUITE, LensFromLambda) {
// From rvalue
{
int storedVal = 3;
auto lens =
Lens<int, int>([](int&& j) -> int& { return j; },
[](int& j, int newVal) -> void { j = newVal - 10; });
ASSERT_EQ(lens.get(3), 3);
lens.set(14, storedVal);
ASSERT_EQ(storedVal, 4);
}

// From lvalue
{
int storedVal = 3;
auto getter = [](int&& j, int&& k) -> int& { return j; };
auto setter = [](int& j, int newVal) { j = newVal - 10; };
auto lens = Lens<int, int, int>(getter, setter);
// This is to test that get can accept an lvalue
ASSERT_EQ(lens.get(storedVal, 10), 3);
lens.set(14, storedVal, 5);
ASSERT_EQ(storedVal, 4);
}
}

struct B {
int height = 0;
};

struct A {
B b;
};

TEST(TEST_SUITE, ComposeLens) {
auto storedVal = A{B{10}};
auto getter = [](A&& a) -> B& { return a.b; };
auto setter = [](B& b, B&& newB) { b = newB; };
auto lens = Lens<B, A>(getter, setter);

ASSERT_EQ(lens.get(storedVal).height, 10);

auto getterComp = [](B&& b) -> int& { return b.height; };
auto setterComp = [](int& height, int&& newHeight) { height = newHeight; };
auto lensComp = Lens<int, B>(getterComp, setterComp);

auto composedLens = lens.compose(lensComp);
ASSERT_EQ(composedLens.get(storedVal), 10);
composedLens.set(20, storedVal);
ASSERT_EQ(composedLens.get(storedVal), 20);
}

TEST(TEST_SUITE, PrecomposeLens) {
auto storedVal = A{B{10}};
auto getter = [](A&& a) -> B& { return a.b; };
auto setter = [](B& b, B&& newB) { b = newB; };
auto lens = Lens<B, A>(getter, setter);

ASSERT_EQ(lens.get(storedVal).height, 10);

auto getterComp = [](B&& b) -> int& { return b.height; };
auto setterComp = [](int& height, int&& newHeight) { height = newHeight; };
auto lensComp = Lens<int, B>(getterComp, setterComp);

auto composedLens = lensComp.precompose(lens);
ASSERT_EQ(composedLens.get(storedVal), 10);
composedLens.set(20, storedVal);
ASSERT_EQ(composedLens.get(storedVal), 20);
}

TEST(TEST_SUITE, CompositionCopiesLenses) {
// Create the lenses
auto getter = [](A&& a) -> B& { return a.b; };
auto setter = [](B& b, B&& newB) { b = newB; };
auto lensAPtr = new Lens<B, A>(getter, setter);

auto getterComp = [](B&& b) -> int& { return b.height; };
auto setterComp = [](int& height, int&& newHeight) { height = newHeight; };
auto lensBPtr = new Lens<int, B>(getterComp, setterComp);

auto composedLens = lensAPtr->compose(*lensBPtr);

// Delete the lenses
delete lensAPtr;
lensAPtr = nullptr;
delete lensBPtr;
lensBPtr = nullptr;

// Use the composed lens
auto storedVal = A{B{10}};
ASSERT_EQ(composedLens.get(storedVal), 10);
auto j = 20;
composedLens.set(j, storedVal);
ASSERT_EQ(composedLens.get(storedVal), 20);
}