Engine: Add optics to core#50
Conversation
|
It's not very clear why there is a need to split the PR here:
Maybe you could integrate lens into the existing get/setting attributes so that it exercised by E2E |
There was a problem hiding this comment.
Reviewed. Honestly this has been really difficult to review.
- Moving forward I hope we can have solutions that are simple & elegant, instead of being complex for the sake for of complex for code readability and maintainability down the road.
- As mentioned in an above comment, Code should be used instead of left hanging unused: without seeing how its used in the Engine, its hard to understand how this code is used, whether this complexity is needed, and whether this code works (should be exercised by E2E tests).
| 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...>(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Possible to convert this into a C++ 17 fold expression?
| : getter(getter) {} | ||
|
|
||
| template <class... RArgs> | ||
| requires(is_same_args<Args...>::template as<RArgs...>()) Return |
There was a problem hiding this comment.
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?
| // does the same thing. | ||
|
|
||
| template <class Return, class... Args> | ||
| class Getter { |
There was a problem hiding this comment.
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_valuesIn that case, maybe selector/prism/transform might be a better name than Getter?
| // Make a copy of the getter so that if the current Getter is destroyed | ||
| // The returned getter is still usable |
There was a problem hiding this comment.
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.
| class Getter { | ||
| protected: | ||
| public: | ||
| const std::function<Return(std::remove_reference_t<Args>&&...)> getter; |
There was a problem hiding this comment.
Naming this function getter is too close to Getter, making the code hard to follow.
| 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; |
There was a problem hiding this comment.
Not sure what the point of making these private is.
| }; | ||
|
|
||
| template <class Return, class... Args> | ||
| class Lens { |
There was a problem hiding this comment.
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 eliminatesetters?.
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.
| && 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)); |
There was a problem hiding this comment.
Consistency. Choose whether you would like to use this-> or not. Earlier getter was used without this->.
| 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) |
There was a problem hiding this comment.
Please use typedef/using so that function declarations are easier to read.
| template <class NewReturn> | ||
| Lens<NewReturn, Args...> compose( | ||
| const Lens<NewReturn, Return>& otherLens) const { | ||
| std::function<NewReturn&(Args && ...)> get = |
There was a problem hiding this comment.
Choose another name. This one overshadows get() the method which might not be intended.
2 participants
This PR adds:
These are needed for array index operation. This PR does not introduce any usage of the optics, but it is intended to be used to replace passing and recreating
ValueingraphInterpreter.cpp.