patterncppMinor
Invoking functions with arguments at a later time or multiple times
Viewed 0 times
argumentslaterwithtimeinvokingtimesmultiplefunctions
Problem
This is a class which stores a function pointer and arguments to that function, to call/invoke that function at a later point in time, or multiple times. Additionally, through polymorphism, it allows methods with the same return type (though potentially different arguments) to be grouped (in a container) and invoked being agnostic to the actual function.
The main areas I'm interested in are correctness (use of const-correctness, move semantics, etc.), ease of use and performance.
The code:
```
#pragma once
#include
#include
/**
* @brief Contains internal details for implementing FunctionInvocation.
*/
namespace detail
{
/**
* @brief Structure whose template arguments represent a sequence
* of numbers. Use @ref SequenceGen to generate such a type.
*/
template
struct Sequence { };
/**
* @brief When given a single template parameter it'll generate a
* @ref Sequence for which the TNumbers is a sequence from
* 0 to TNumber (inclusive).
*
* @param TNumber The number towards which the sequence should
* count (inclusive).
*
* @remark You cannot pass in a negative number.
*/
template
struct SequenceGen;
template
struct SequenceGen
{
/**
* @brief The type of the sequence.
*
* Will contain a sequence from 0 to TNumber (inclusive).
* For example, SequenceGen::Type will return Sequence.
*/
using Type = Sequence;
};
template
struct SequenceGen : SequenceGen
{
static_assert(TCurrentNumber > 0, "Sequence Generator can only take positive integers.");
};
/**
* @brief Helper function to invoke a method with arguments supplied through a tuple.
*/
template
inline TReturn InvocationHelper(TReturn(*function)(TArgs...), const std::tuple& arguments, Sequence)
{
// Invoke the function with a copy of all items in the
The main areas I'm interested in are correctness (use of const-correctness, move semantics, etc.), ease of use and performance.
The code:
```
#pragma once
#include
#include
/**
* @brief Contains internal details for implementing FunctionInvocation.
*/
namespace detail
{
/**
* @brief Structure whose template arguments represent a sequence
* of numbers. Use @ref SequenceGen to generate such a type.
*/
template
struct Sequence { };
/**
* @brief When given a single template parameter it'll generate a
* @ref Sequence for which the TNumbers is a sequence from
* 0 to TNumber (inclusive).
*
* @param TNumber The number towards which the sequence should
* count (inclusive).
*
* @remark You cannot pass in a negative number.
*/
template
struct SequenceGen;
template
struct SequenceGen
{
/**
* @brief The type of the sequence.
*
* Will contain a sequence from 0 to TNumber (inclusive).
* For example, SequenceGen::Type will return Sequence.
*/
using Type = Sequence;
};
template
struct SequenceGen : SequenceGen
{
static_assert(TCurrentNumber > 0, "Sequence Generator can only take positive integers.");
};
/**
* @brief Helper function to invoke a method with arguments supplied through a tuple.
*/
template
inline TReturn InvocationHelper(TReturn(*function)(TArgs...), const std::tuple& arguments, Sequence)
{
// Invoke the function with a copy of all items in the
Solution
Reinventing the wheel
What you're writing is basically a strictly worse version of
Rule of Zero
Your class has two members: A
You'll need to default the constructor:
But then for the other special member functions, you should either default all of them or omit all of them:
Prefer omission.
Construction
You are allowing your arguments to only be constructed by rvalue reference.
The
IsEmpty
This function should be named
Template Signature
You are currently just listing all your types in order. However, if you look at all the other class templates that take a callable, they always take a single template argument as the signature. This makes the usage more explicit. That is:
Polymorphism
Having virtual polymorphism isn't useful here, since then the user has to keep track of allocated memory. I'd want to be able to write something like:
And have that handle the type erasure. I don't want to have to write:
since then I'd have to write
which, going back to my initial point, is exactly what I would have to write with the standard library:
Sequence
Comment Style
It's great that you're writing lots of comments, but this is a truly excessive amount of comments. There is an enormous amount of grey text here, and it actually didn't help me understand anything hardly at all. Do you need to write a 5-line comment on what the copy constructor does? It's a copy constructor. Do you need an 8-line comment on move assignment? It's move assignment.
What you're writing is basically a strictly worse version of
bind. It's strictly worse for two reasons. First, it only allows for function pointers. You can't use member functions or callable objects. Second, you can't partially apply functions. It's all or nothing. So, it's not nearly as useful as it could be. If the goal of this exercise is simply to reimplement bind, it's definitely worth trying to support both of those features.Rule of Zero
Your class has two members: A
std::tuple and a function pointer. These are already copyable and movable where appropriate. As such, writing your own operations is at best, a waste of code and at worst, error-prone.You'll need to default the constructor:
std::tuple _args;
Delegate _fn = nullptr;
StoredFunctionInvocation() = default;But then for the other special member functions, you should either default all of them or omit all of them:
~StoredFunctionInvocation() = default;
StoredFunctionInvocation(const StoredFunctionInvocation&) = default;
StoredFunctionInvocation(StoredFunctionInvocation&& ) = default;
StoredFunctionInvocation& operator=(const StoredFunctionInvocation&) = default;
StoredFunctionInvocation& operator=(StoredFunctionInvocation&&) = default;Prefer omission.
Construction
You are allowing your arguments to only be constructed by rvalue reference.
TArgs&&... is not a forwarding reference since TArgs is not a template argument of the function. As such, the forward() call is really just a move(). Prefer:template (Us&&...)>::value>
>
StoredFunctionInvocation(Delegate function, Us&&... args)
: _fn(function)
, _args(std::forward(args)...)
{ }The
inline is unnecessary. IsEmpty
This function should be named
explicit operator bool() const for consistency with other similar container-like objects.Template Signature
You are currently just listing all your types in order. However, if you look at all the other class templates that take a callable, they always take a single template argument as the signature. This makes the usage more explicit. That is:
StoredFunction invoc1(fn1, 1, 2);Polymorphism
Having virtual polymorphism isn't useful here, since then the user has to keep track of allocated memory. I'd want to be able to write something like:
FunctionInvocation invoc1 = StoredFunction(fn1, 1, 2);And have that handle the type erasure. I don't want to have to write:
FunctionInvocation* invoc1 = new StoredFunction(fn1, 1, 2);since then I'd have to write
(*invoc1)(), which is quite unnatural. While we're add it, I don't want to have to spell out StoredFunction(fn1, 1, 2) either and would much rather be able to write:FunctionInvocation invoc1 = StoreFunction(fn1, 1, 2);which, going back to my initial point, is exactly what I would have to write with the standard library:
std::function invoc1 = std::bind(fn1, 1, 2);Sequence
SequenceGen should generate the sequence `. That will make it much more useful. Otherwise, you will end up calling SequenceGen a lot.
Also, you should provide an alias template from SequenceGen, so that you can simply write:
return InvocationHelper(_fn, _args, make_sequence{});
You should not need to specify the template arguments here as they should be deducible from _fn and _args.
InvocationHelper
Why are you invoking the function with a copy of all of the items? You're already copying them once (into the tuple`). Why copy them again? Comment Style
It's great that you're writing lots of comments, but this is a truly excessive amount of comments. There is an enormous amount of grey text here, and it actually didn't help me understand anything hardly at all. Do you need to write a 5-line comment on what the copy constructor does? It's a copy constructor. Do you need an 8-line comment on move assignment? It's move assignment.
Code Snippets
std::tuple<TArgs...> _args;
Delegate _fn = nullptr;
StoredFunctionInvocation() = default;~StoredFunctionInvocation() = default;
StoredFunctionInvocation(const StoredFunctionInvocation&) = default;
StoredFunctionInvocation(StoredFunctionInvocation&& ) = default;
StoredFunctionInvocation& operator=(const StoredFunctionInvocation&) = default;
StoredFunctionInvocation& operator=(StoredFunctionInvocation&&) = default;template <typename... Us,
typename = std::enable_if_t<std::is_constructible<
std::tuple<TArgs...>(Us&&...)>::value>
>
StoredFunctionInvocation(Delegate function, Us&&... args)
: _fn(function)
, _args(std::forward<Us>(args)...)
{ }StoredFunction<int(int, int)> invoc1(fn1, 1, 2);FunctionInvocation<int()> invoc1 = StoredFunction<int(int, int)>(fn1, 1, 2);Context
StackExchange Code Review Q#108550, answer score: 4
Revisions (0)
No revisions yet.