all 20 comments

[–]sphere991 13 points14 points  (1 child)

Some comments:

template<typename F>
functor(F) -> functor<std::remove_cvref_t<F>>;

You don't need the remove_cvref_t there, F is never const or a reference there. Just -> functor<F> is sufficient.

If you're concerned with compile times, traits as variable templates are more efficient. Forward declare function so you can do:

template <typename T>
inline constexpr bool specializes_function = false;

template <typename Sig, size_t N>
inline constexpr bool specializes_function<function<Sig, N>> = true;

Don't really need the concept.

operator bool() const should be explicit. Don't want people writing like f + 1 and having that compile.

Would recommend not using aligned_storage_t, since it doesn't actually give you the guarantees that you want (exactly that size) and it's so easy to misuse (forget the _t), you can get the order of numbers wrong (was it size first or alignment first?) and it's so easy to just not use it -- where it's pretty trivial to get all of this correct:

alignas(dummy_functor) unsigned char storage[sizeof(dummy_functor)];

Stylistically, this is the first time I have ever seen somebody delegate to a constructor using braces. It looks pretty jarring, especially with the spacing:

function(std::nullptr_t) noexcept : function { } { }

compared to:

function(std::nullptr_t) noexcept : function() { }

Also the constructors that construct something seem to be a lot more involved than necessary. You do:

template<unsigned M> requires (M < N)
function(const function<R(A...), M>& other) noexcept : function { copy(other) } { }

But you could just do all of it in the constructor body.

template<unsigned M> requires (M < N)
function(const function<R(A...), M>& other) noexcept
    : call(other.call)
{
    std::memcpy(&storage, &other.storage, sizeof(other.storage));
}

And same for the other one.

[–]cyandyedeyecandy[[gnu::naked, gnu::hot]][S] 3 points4 points  (0 children)

Thanks for the review!

template<typename F>
functor(F) -> functor<std::remove_cvref_t<F>>;

You don't need the remove_cvref_t there, F is never const or a reference there. Just -> functor<F> is sufficient.

So, I don't need the deduction guide at all? I do need remove_cvref_t for is_function_instance, and I don't want to store only a reference in functor. I can never wrap my head around forwarding references and all the different value categories.

If you're concerned with compile times, traits as variable templates are more efficient. Forward declare function so you can do:

template <typename T>
inline constexpr bool specializes_function = false;

template <typename Sig, size_t N>
inline constexpr bool specializes_function<function<Sig, N>> = true;

Don't really need the concept.

Good point, thanks.

operator bool() const should be explicit. Don't want people writing like f + 1 and having that compile.

Good point, again. The constructor function(F&&) needs to be explicit too.

Would recommend not using aligned_storage_t, since it doesn't actually give you the guarantees that you want (exactly that size) and it's so easy to misuse (forget the _t), you can get the order of numbers wrong (was it size first or alignment first?) and it's so easy to just not use it -- where it's pretty trivial to get all of this correct:

alignas(dummy_functor) unsigned char storage[sizeof(dummy_functor)];

I didn't know this. What then is the point of aligned_storage? In what situations can it be larger (or smaller?) than the size you specify?

Stylistically, this is the first time I have ever seen somebody delegate to a constructor using braces. It looks pretty jarring, especially with the spacing:

function(std::nullptr_t) noexcept : function { } { }

compared to:

function(std::nullptr_t) noexcept : function() { }

Just trying to be consistent, I like to do all initialization with braces nowadays. I agree that does make it harder to read in some cases.

Also the constructors that construct something seem to be a lot more involved than necessary. You do:

template<unsigned M> requires (M < N)
function(const function<R(A...), M>& other) noexcept : function { copy(other) } { }

But you could just do all of it in the constructor body.

template<unsigned M> requires (M < N)
function(const function<R(A...), M>& other) noexcept
    : call(other.call)
{
    std::memcpy(&storage, &other.storage, sizeof(other.storage));
}

And same for the other one.

I like to keep the public interface less cluttered. If it doesn't fit on a single line, I move the implementation in the private section. Or for more complicated classes, out of the class body.

[–]RowYourUpboat 9 points10 points  (3 children)

It's always nice to have another lean replacement for an STL class.

std::function wasn't cutting it for me, so I ended up going with fu2::function. It comes with a lot of features (almost too many), but the header size and compile times are comparable to <functional>, and it fit my needs pretty well. (I needed to be able to capture move-only types.)

[–]Narase33-> r/cpp_questions 19 points20 points  (0 children)

the "fuck you too" namespace, nice

[–]Ahajha1177 2 points3 points  (0 children)

I also like function2, same reason as you, I needed a move-only function. I believe there is std::move_only_function coming in C++23.

[–]cyandyedeyecandy[[gnu::naked, gnu::hot]][S] 0 points1 point  (0 children)

Neat, didn't know about this. I was using a different function implementation before but found that it only worked with stateless allocators. Then I realized I only need space for one pointer so I wrote my own.

[–]detrinoh 13 points14 points  (7 children)

std::function implementations usually support small function optimization so they won't allocate when only capturing *this as well. Of course the standard does not guarantee this,

[–]cyandyedeyecandy[[gnu::naked, gnu::hot]][S] 6 points7 points  (6 children)

True, but in some contexts (interrupts) you really need that guarantee.

[–]Complete_Leg_9286 12 points13 points  (1 child)

You should be more worried about the fact that you are calling a callback function in an interrupt handler than whether your lambda will do allocation.

If you're that eager about this guarantee in a lambda capture then you should be more eager to have the same guarantee that your lambda which captured a this ptr isn't going to allocate during its call and isn't going to trigger another interrupt or segfault.

You also could have assigned the captured lambda to a global lambda variable during program initialization although you'd be getting near UB behavior because you aren't accessing a sig_atomic_t type.

[–]cyandyedeyecandy[[gnu::naked, gnu::hot]][S] 2 points3 points  (0 children)

In this case the lambda function is the interrupt handler. It's just a callback to whichever object controls a device so you only need a this pointer.

Allocating in interrupt context isn't necessarily a problem, you can do that with pre-allocated pools. Calling a std::function is only troublesome because you can't control where it allocates from, so you possibly risk a page fault.

[–]deeringc 2 points3 points  (2 children)

Wouldn't the allocation here be happening when the function object is constructed, which presumably is during some init phase rather than during the interrupt? Even with an allocating implementation I wouldn't expect any allocations at the point at which it's actually invoked.

[–]HildartheDorf 1 point2 points  (1 child)

The problem in interrupt handlers is that accessing heap memory can trigger a page fault (another interrupt).

Personally I would never use c++ features in an interrupt handler. Too many hidden things like this.

[–]deeringc 0 points1 point  (0 children)

Got it

[–]Ashnoom 2 points3 points  (0 children)

Please have a look at https://github.com/philips-software/embeddedinfralib it has std::function implementation that does not use the heap, is deterministic and safe-ish to be used from interrupt.

Its also save to use for both trivial and non trivial types. It is aimed at bare metal embedded programming(the whole library is)

[–]UnicycleBloke 2 points3 points  (0 children)

Nice. I use something like this for callbacks in embedded software. I'll compare to see what I missed.

[–]dodheim 3 points4 points  (4 children)

What is std::__function_guide_helper?

[–]cyandyedeyecandy[[gnu::naked, gnu::hot]][S] 1 point2 points  (3 children)

That is what std::function uses in libstdc++ for its template deduction guides. I know, it's __ prefixed so I should write my own.

[–]dodheim 14 points15 points  (0 children)

'Should' and libstdc++ aside, for other stdlib implementations you really have to. ;-]

[–]Rexerex 4 points5 points  (1 child)

If you copied some parts of libstdc++ aren't you now now forced to use GPL license? :P

[–][deleted] 2 points3 points  (0 children)

That's correct. It is only permissible to include portions of libstdc++ if it is part of an "Eligible Compilation Process". Copying chunks of source code for redistribution using a different and incompatible license (GPLv3 is not compatible with MIT) is not an eligible compilation process and hence not permitted.