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.