Allow `std::string_view` like behaviour for temporary lambda captures

[duckdoom5]

Hi I was trying out this library but I ran into a problem.
Maybe it's just me not understanding the intended use case or maybe there's something else going wrong.

I tried to use std::function_ref as a drop-in replacement for parameters that used to be template args.
The code below shows a simplified version of what I attempted to do:

namespace {
    class CallbackStore {
    public:
        using CallbackSig = void(int);

        void AddCallback(const std23::function_ref<CallbackSig>& action) {
            _actions.emplace_back(action);
        }

        void Invoke(int args) const {
            for (const std23::function<CallbackSig>& f : _actions)
                f(args);
        }

    private:
        std::vector<std23::function<CallbackSig>> _actions;
    };

    CallbackStore g_store;
} // namespace

AppMain::AppMain() {
    g_store.AddCallback([&](int i) {
        OnCallback(i);
    });
}

void AppMain::OnInitialize() {
    g_store.AddCallback([&](int i) {
        OnCallback(i);
    });
    g_store.Invoke(2);
}

void AppMain::OnCallback(int i) {
    std::cout << std::format("Callback value: {}\n", i);
}

This isn't a 100% replicate-able example, but I hope it does suffice.

So the issue is that the first callback in the AppMain() constructor has a 'corrupted' this pointer.
(Note: I placed a breakpoint before the callback was added to capture the address of the AppMain class to display that it has not moved)

The second callback works fine.

If I understand lambdas correctly, I assume what happens here is that the compiler generated lambda captured data storage is deleted before the callback is invoked. Is this just a bug or is the way I implemented it simply incorrect?

[zhihaoy]

The parameter was taken as const std23::function_ref<CallbackSig>& but saved as std23::function<CallbackSig>. Doing so does not help with lifetime issues, because if function_ref referenced something out of the scope, function merely contains a copy of that function_ref which referenced something out of the scope. If you do want to potentially store the callable targets, just take the parameter std23::function<CallbackSig> and std::move into the vector. If you do not (at the cost explained below), you should accept the argument as std23::function_ref<CallbackSig> and store std23::function_ref<CallbackSig> as well.

After making the above change, I suggest trying this:

    g_store.AddCallback({std23::nontype<&AppMain::OnCallback>, this});

If you store a vector of std23::function_ref, other forms of callbacks that do need storage (such as lambda with captures) will be dead on arrival, but {std23::nontype<&AppMain::OnCallback>, this} is okay, and was one of the main purposes of this project. Otherwise, accept and store std23::function (or std23::move_only_function).

[duckdoom5]

Makes sense. Thanks for the explanation 👍🏼

[duckdoom5]

One more question. If I now try to use std23::move_only_function, MSVC gives me the following error:

'std23::move_only_function<void (const AppMain &,int),void (const AppMain &,int)>::operator ()': no overloaded function has valid conversion for 'this' pointer.

Is that also ' correct' (aka as designed, it should not work on member functions)?

With that in mind, would this comment from SO still apply to std23::function or is that only relevant when you do not store the function? I learned from my peers that you should always pass functions as templates like this comment mentioned.

[zhihaoy]

One more question. If I now try to use std23::move_only_function, MSVC gives me the following error:

'std23::move_only_function<void (const AppMain &,int),void (const AppMain &,int)>::operator ()': no overloaded function has valid conversion for 'this' pointer.

Is that also ' correct' (aka as designed, it should not work on member functions)?

Your CallbackSig was void(int) not void (const AppMain &,int); the former is more likely to be correct. Please start from https://godbolt.org/z/3aqhcnvcb if you would like to share a minimal-reproduce.

W.r.t. std::move_only_function, as a new addition to C++23, it is deeply const-correct. "Constness" of the function signature (the const in move_only_function<void (int) const> if you want to give it a try), and the constness of move_only_function object itself both have some guardrails in play. It's not only a move-only C++11 std::function.

With that in mind, would this comment from SO still apply to std23::function or is that only relevant when you do not store the function? I learned from my peers that you should always pass functions as templates like this comment mentioned.

If you're only calling a function soon in the scope where the function is taken as a parameter, forwarding reference provides more reasonable codegen compared to std(23)::function, but it doesn't mean that its performance is a holy grail, and there are other forms of cost as well. std(23)::function_ref should be the first option to consider (as its name suggests, function's reference). function_ref's codegen maybe even more appropriate (often, code snippet is still inlined, only the boundary of inlining is shifted), plus various kinds of programantic benefits.

[duckdoom5]

Hi, sorry for the long wait. I had some stuff going on.

Anyway. I've build a repro here which is close to my real world usecase.
I applied your previous suggestion to accept and store std23::function.

The example shows that we get a crash when we accept a const std23::function& as param and then pass it by reference to another lambda.
Is there a way to make this safe(r) so we get a compile time error instead of a runtime one?

Now, honestly, this makes complete sense to me, but given that it does work with std::function I thought I'd bring it up
Ha, never mind. It also crashes with std::function 😅.

I also attempted to convert it to move_only_function again. I guess I just don't quite understand which version to use when in this case:
https://godbolt.org/z/GoE5hEYPs

[zhihaoy]

The example shows that we get a crash when we accept a const std23::function& as param and then pass it by reference to another lambda. It there a way to make this safe(r) so we get a compile time error instead of a runtime one?

The crash cause is slightly more complicated. You meant to store a callback and call it, but the actual code stores a callback and does not call the stored callback. It's not only a lifetime issue, but some compiler extensions may help. Some research is needed.

Now, honestly, this makes complete sense to me, but given that it does work with std::function I thought I'd bring it up Ha, never mind. It also crashes with std::function 😅.

I also attempted to convert it to move_only_function again. I guess I just don't quite understand which version to use when in this case: https://godbolt.org/z/GoE5hEYPs

A small issue,

-        using CallbackSig = void(Args...);
+        using CallbackSig = void(Args...) const;

https://godbolt.org/z/TdY6Yecqa

because std::move_only_function doesn't provide const call operator if its signature is not a const call signature (std::function is considered broken because its call operator is always const, and std23::function in this repo is a compliant, bug-per-bug implementation).

I also showed how to capture move_only_function by value in this snippet. It's good that storage was provided for add and _actions, but as you already know, the fact that the action to be called references the temporary object materialized on the right-hand side of OnCollisionRef += bypasses that. Capturing action by value is a way to store a copy of action in _actions. But then we come to the question: why the .add callback isn't as simple as

            .add = [=](CollisionEventHandler&& action) {
                return characterControllerPtr->OnCollision += std::move(action);
            },

https://godbolt.org/z/9bToKzvfv

Of course, it's entirely possible that you wanted to add extra code before or after calling action:

                return characterControllerPtr->OnCollision += [/* what goes here? */](EventArgs& args)
                {
                    // some extra work here
                    action(args);
                };

and this is when std23::nontype shins. It looks at the problem from a different angle:

                return characterControllerPtr->OnCollision += {
                    std23::nontype<[](auto& self, EventArgs& args) {
                        std::clog << "some extra work\n";
                        self(args);
                    }>,
                    std::move(action)};

https://godbolt.org/z/9Pd8q3fET

Compare the above with

            .add = [=](CollisionEventHandler&& action) {
                return characterControllerPtr->OnCollision += std::move(action);
            },

You'll find that nontype allows you to replace the "supposed" call operator of the callable, leaving lifetime management of the object to be called unchanged.

[duckdoom5]

Of course, it's entirely possible that you wanted to add extra code before or after calling action

Yeah, that was the goal in the original code. There I do a pass on the EventArgs, converting them from physics engine implementation data to user facing data representation. I stripped out the custom event args structures to make the example a bit more simple, but didn't consider that it might look a bit strange due to that :P

and this is when std23::nontype shins. It looks at the problem from a different angle:

return characterControllerPtr->OnCollision += {
    std23::nontype<[](auto& self, EventArgs& args) {
        std::clog << "some extra work\n";
        self(args);
    }>,
     std::move(action)};

Ah I love that! Awesome

Anyway, thanks again for taking the time to explain it to me. Makes a lot of sense now