Feature functional tooling start - #74
Conversation
|
I have three different mechanisms for implementing a small set of functional tools. I want to get input on these and discuss what the best way forward is. I think the 3 parameter functions has the least promise. These were the simplest to implement because they just use normal template parameter deduction, but specify the type as a parameter also the value of that parameter to matter. for now it just appends to the output set of each function. This isn't idiomatic but could be useful if we go this way. The 2 parameter functions like the current Convert and Reject work, and are simple. They can be combined in interesting ways with the currying functions to achieve just about anything. I think nesting a bunch of function can lead to gross syntax and I think that is likely with this. The 2 parameter select function has a bunch of gross machinery behind it. It should be able to fully interoperate with the two parameter functions however we want, but it can also curry itself. If given a predicate and no data it will return a function that accepts data and performs the previously setup selection. This has the drawback of being a bit complex, but I ability to curry without needing to explicitly call a seperate function makes it easier to construct a cleaner syntax for chaining transformations. I am experimenting with this in the Pipe class, which mostly noise right now. |
|
I know this is really rough, most of it can be ignored. The main work and experiments are in "FunctionalTools.h" and "FunctionalToolsTests.h" |
MakoEnergy
left a comment
MakoEnergy
left a comment
There was a problem hiding this comment.
Finished my initial pass. Aside from the obvious stuff of needing to document and wondering where you are going with the piping...it looks good thus far.
MakoEnergy
left a comment
MakoEnergy
left a comment
There was a problem hiding this comment.
Finished second pass of the current state of the code.
| decltype(auto) Name(const CVIncomingContainerType& IncomingContainer) \ | ||
| { \ | ||
| return StructName{}(std::forward<const CVIncomingContainerType&>(IncomingContainer)); \ | ||
| } \ |
There was a problem hiding this comment.
You aren't using a universal reference for the parameter here, so I don't think you need the forward.
There was a problem hiding this comment.
Yeah, I need to spend time cleaning this kind of noise up. This is from repeated tests/refactors and never cleaning it up.
| decltype(auto) Name(const CVIncomingContainerType& IncomingContainer, PredicateType Predicate) \ | ||
| { \ | ||
| return StructName<>{} \ | ||
| (std::forward<const CVIncomingContainerType&>(IncomingContainer), \ | ||
| std::forward<PredicateType>(Predicate)); \ | ||
| } \ |
There was a problem hiding this comment.
Like below, no need for forward when not using universal reference.
| START_FUNCTOR_END_CONTAINER_TO_DATA_AUTOCURRY(MinStruct) | ||
| template<typename CVIncomingContainerType, typename PredicateType> | ||
| [[nodiscard]] constexpr | ||
| decltype(auto) operator() (CVIncomingContainerType&& IncomingContainer, PredicateType Predicate) const |
There was a problem hiding this comment.
I'm not sure which is intentional, but it's odd that you are passing predicate by const reference in the function above this one, but by value in this one. AND you are forwarding this one (which again, no universal reference so no need).
There was a problem hiding this comment.
I will try to change the predicates to universal references.
| using IncomingContainerType = std::decay_t<CVIncomingContainerType>; | ||
| using namespace ContainerDetect; | ||
| static_assert(IsRange<IncomingContainerType>(), | ||
| "Mezzanine::MinMax only determines the Min from containers or ranges."); |
There was a problem hiding this comment.
I believe this message should say "Mezzanine::Min" instead of "MinMax".
| auto Results = std::minmax_element(IncomingContainer.cbegin(), IncomingContainer.cend()); | ||
| return std::pair{*Results.first, *Results.second}; | ||
| } | ||
| else | ||
| { | ||
| auto Results = std::minmax_element(IncomingContainer.cbegin(), IncomingContainer.cend(), Predicate); | ||
| return std::pair{*Results.first, *Results.second}; |
There was a problem hiding this comment.
/nit Is use of std::make_pair appropriate here?
| namespace Mezzanine | ||
| { | ||
| namespace Functional { |
There was a problem hiding this comment.
/nit I find your lack of consistency....disturbing.
|
|
||
| // Convert this when assiging to strongly typed storage. | ||
| [[nodiscard]] constexpr | ||
| operator auto() const |
There was a problem hiding this comment.
Type is cleanly known. No need for "auto".
| if constexpr( HasReserve<ReturnContainerType>()) | ||
| { Results.reserve(HowMany); } | ||
|
|
||
| for(SizeType Counter{0}; Counter<HowMany; Counter++, Copying++) |
There was a problem hiding this comment.
/nit Some spaces around the operator for visibility would be nice.
| if constexpr( HasReserve<ReturnContainerType>()) | ||
| { Results.reserve(HowMany); } | ||
|
|
||
| for(SizeType Counter{0}; Counter<HowMany; Counter++, Copying++) |
| if constexpr(HasPushBackValue<ReturnContainerType>()) | ||
| { Results.push_back(*Copying); } | ||
| else if constexpr(HasInsertValue<ReturnContainerType>()) | ||
| { Results.insert(*Copying); } | ||
| else if constexpr(HasAddValue<ReturnContainerType>()) | ||
| { Results.add(*Copying); } |
There was a problem hiding this comment.
For all of the operations that copy the elements, should we static_assert to verify the element is copy constructable? I also have the slightest bit of concern about situations where a moving variant of these functions may be selected instead of the copy.
There was a problem hiding this comment.
These are const iterators pointing into a const container, moving shouldn't be possible.
noexcept in test lambdas removing non-standard conversion based on auto.
2 participants
This PR isn't complete yet, but when done it will include a basic set of tools for functional programming.