remove eldritch preprocessor code from DataFuncs.h

[chdoc]

No description provided.

[ab9rf]

so far after a moderate amount of testing this code both appears to work and appears to have performance comparable to what we have in develop. there is some indication that it is slightly less performant, but if it is, the difference is extremely small. going to do some more focused performance testing in the near future

[lethosor]

Thanks for taking this on. I've wanted to generalize the macros for several years now.

[lethosor]

Ideally this would be in LuaTools.h, or not in DFHack::Lua. We already have enough headers where bits of DFHack::Lua are defined.

[ab9rf]

i concur, there doesn't seem to be a reason why this needs to be in the DFHack::Lua namespace

[chdoc]

I agree that it would be nice to have this in LuaTools.h. This is the reason why I put it in the namespace where it currently is. The reason I didn't put it to the right header immediately is that this would require DataFuncs to import LuaTools.h and IIRC LuaTools.h to include DataIdentity.h , and I wasn't sure about the relationship between the various Lua headers.

[lethosor]

Consistent capitalization would be best. I think I prefer all uppercase, so RT and AT in addition to CT

[ab9rf]

also allow me to here express my strong preference for using using A = B; instead of typedef B A;

[chdoc]

Both or fine with me, of course. I just copied the existing typedef, and aT/rT just happens to be a convention that I'm used to from a different, non C++, library.

[lethosor]

This feels to me like it could be a function instead of a macro, but I haven't tried it.

[ab9rf]

the first argument is a type, so it would have to be a templated function as types are not first class objects

[ab9rf]

seems you're right:

    template<typename RT, typename FT, typename... AT>
        requires std::is_invocable_v<FT, AT...>
    void call_and_push(lua_State* L, FT fun, std::tuple<AT...>& a)
    {
        if constexpr (std::is_same_v<RT,void>){
            std::apply(fun,a);
        }
        else
        {
            RT rv = std::apply(fun, a);
            df::identity_traits<RT>::get()->lua_read(L, UPVAL_METHOD_NAME, &rv);
        }
    }

#define CALL_AND_PUSH(rT,fun,args) call_and_push<rT>(L, fun, args);

requires type_traits header to bring in is_invocable; can't just use a function type here because fun might be a pointer to a member, which isn't a function but is invocable

may also be possible to get rid of the explicit tuple by using std::invoke instead of std::apply and making the function variadic - indeed yes:

    template<typename RT, typename FT, typename... AT>
        requires std::is_invocable_r_v<RT, FT, AT...>
    void call_and_push(lua_State* L, FT fun, AT... a)
    {
        if constexpr (std::is_same_v<RT,void>){
            std::invoke(fun,a...);
        }
        else
        {
            RT rv = std::invoke(fun, a...);
            df::identity_traits<RT>::get()->lua_read(L, UPVAL_METHOD_NAME, &rv);
        }
    }

    template<typename T> struct function_wrapper {};

    template<typename rT, typename ...aT>
    struct function_wrapper<rT(*)(DFHack::color_ostream&, aT...)> {
        static const int num_args = sizeof...(aT);
        static void execute(lua_State *L, int base, rT (fun)(DFHack::color_ostream& out, aT...)) {
             cur_lua_ostream_argument out(L);
             call_and_push<rT>(L, fun, out, (DFHack::Lua::Get<aT>(L, base++))... );
        }
    };

    template<typename rT, typename ...aT>
    struct function_wrapper<rT(*)(aT...)> {
        static const int num_args = sizeof...(aT);
        static void execute(lua_State *L, int base, rT (fun)(aT...)) {
            call_and_push<rT>(L, fun, (DFHack::Lua::Get<aT>(L, base++))...);
        }
    };

    template<typename rT, class CT, typename ...aT>
    struct function_wrapper<rT(CT::*)(aT...)> {
        static const int num_args = sizeof...(aT)+1;
        static void execute(lua_State *L, int base, rT(CT::*mem_fun)(aT...)) {
            CT *self = (CT*)DFHack::LuaWrapper::get_object_addr(L, base++, UPVAL_METHOD_NAME, "invoke");
            call_and_push<rT>(L, mem_fun, self, (DFHack::Lua::Get<aT>(L, base++))...);
        };
    };

    template<typename rT, class CT, typename ...aT>
    struct function_wrapper<rT(CT::*)(aT...) const> {
        static const int num_args = sizeof...(aT)+1;
        static void execute(lua_State *L, int base, rT(CT::*mem_fun)(aT...) const) {
            CT *self = (CT*)DFHack::LuaWrapper::get_object_addr(L, base++, UPVAL_METHOD_NAME, "invoke");
            call_and_push<rT>(L, mem_fun, self, (DFHack::Lua::Get<aT>(L, base++))...);
        };
    };

(updated to use is_invocable_r which asserts the return type as well as the arguments of the invocable object)

[ab9rf]

i also note that switching it to use std::invoke instead of std::apply (as above) would eliminate the explicit tuple, which should also resolve my (slight) concerns about performance

[ab9rf]

i wasn't happy with the DRY violation in the last round, so here's this further improvement:

    template<typename RT, typename... AT, typename FT, typename... ET>
        requires std::is_invocable_r_v<RT, FT, ET..., AT...>
    void call_and_push(lua_State* L, int base, FT fun, ET... extra)
    {
        if constexpr (std::is_same_v<RT,void>){
            std::invoke(fun, extra..., (DFHack::Lua::Get<AT>(L, base++))...);
        }
        else
        {
            RT rv = std::invoke(fun, extra..., (DFHack::Lua::Get<AT>(L, base++))...);
            df::identity_traits<RT>::get()->lua_read(L, UPVAL_METHOD_NAME, &rv);
        }
    }

    template<typename T> struct function_wrapper {};

    template<typename rT, typename ...aT>
    struct function_wrapper<rT(*)(DFHack::color_ostream&, aT...)> {
        static const int num_args = sizeof...(aT);
        static void execute(lua_State *L, int base, rT (fun)(DFHack::color_ostream& out, aT...)) {
             cur_lua_ostream_argument out(L);
             call_and_push<rT, aT...>(L, base, fun, out);
        }
    };

    template<typename rT, typename ...aT>
    struct function_wrapper<rT(*)(aT...)> {
        static const int num_args = sizeof...(aT);
        static void execute(lua_State *L, int base, rT (fun)(aT...)) {
            call_and_push<rT, aT...>(L, base, fun);
        }
    };

    template<typename rT, class CT, typename ...aT>
    struct function_wrapper<rT(CT::*)(aT...)> {
        static const int num_args = sizeof...(aT)+1;
        static void execute(lua_State *L, int base, rT(CT::*mem_fun)(aT...)) {
            CT *self = (CT*)DFHack::LuaWrapper::get_object_addr(L, base++, UPVAL_METHOD_NAME, "invoke");
            call_and_push<rT, aT...>(L, base, mem_fun, self);
        };
    };

    template<typename rT, class CT, typename ...aT>
    struct function_wrapper<rT(CT::*)(aT...) const> {
        static const int num_args = sizeof...(aT)+1;
        static void execute(lua_State *L, int base, rT(CT::*mem_fun)(aT...) const) {
            CT *self = (CT*)DFHack::LuaWrapper::get_object_addr(L, base++, UPVAL_METHOD_NAME, "invoke");
            call_and_push<rT, aT...>(L, base, mem_fun, self);
        };
    };

yes, there are two parameter packs in the template. ain't C++ fun?

sadly, i haven't found a way to get rid of the remaining DRY violation in call_and_push, because C++ just won't let you have a variable of type void. pshaw!

[ab9rf]

this replacement for call_and_push uses std::index_sequence to avoid having to use postincrement and thus avoids the sequence point issue

    template<typename RT, typename... AT, typename FT, typename... ET, std::size_t... I>
        requires std::is_invocable_r_v<RT, FT, ET..., AT...>
    void call_and_push_impl(lua_State* L, int base, std::index_sequence<I...>, FT fun, ET... extra)
    {
        if constexpr (std::is_same_v<RT, void>) {
            std::invoke(fun, extra..., (DFHack::Lua::Get<AT>(L, base+I))...);
        }
        else
        {
            RT rv = std::invoke(fun, extra..., (DFHack::Lua::Get<AT>(L, base+I))...);
            df::identity_traits<RT>::get()->lua_read(L, UPVAL_METHOD_NAME, &rv);
        }
    }

    template<typename RT, typename... AT, typename FT, typename... ET, typename indices = std::index_sequence_for<AT...> >
        requires std::is_invocable_r_v<RT, FT, ET..., AT...>
    void call_and_push(lua_State* L, int base, FT fun, ET... extra)
    {
        call_and_push_impl<RT, AT...>(L, base, indices{}, fun, extra...);
    }

[chdoc]

And I was thinking that I was becoming a template wizard. It looks like I still have long way to go.

std::invoke(fun, extra..., (DFHack::Lua::Get<AT>(L, base+I))...);

Three packs, two of them expanded in sync, no explicit tuple. I like this!

Should those two functions be declared inline? I suppose that compilers will probably inline them regardless, but maybe we should nevertheless declare that.

@ab9rf Did you check whether the performance impact disappears with this version?

[ab9rf]

I profiled the variation I have here locally and it was intermediate between baseline develop and your first proposal. I remain unconvinced that there is any performance impact; when I drill down into the numbers, there's too much variability in the timings of functions that should not be impacted by the changes for me to be convinced that there is a causal relationship and that the differences between the runs aren't just noise. A better approach to evaluating the performance impact would be to set up a bespoke harness to exercises this code specifically. Sadly, I'm not enough of a Lua programmer to come up with something to do this.

Functions declared within a class definition are always implicitly inline so specifying inline here is redundant.

[ab9rf]

the last two pushes from me ensure correct handling of void functions and address comments raised during code review which should bring this PR closer to our house style (such as it is)

[chdoc]

I have been playing on this branch for several days, and I haven't had a crash. Whatever crashes I had before, they were either resolved by the cleanup from @ab9rf or - more likely - completely unrelated.

Is there anything that is still keeping this from being merged?

[ab9rf]

The issue right now with merging this is that we're in a beta cycle now and because of that we're going to avoid making major product changes not necessary to support the beta.