HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Invoking functions with arguments at a later time or multiple times

Submitted by: @import:stackexchange-codereview··
0
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

Solution

Reinventing the wheel

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.