pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.08k stars 2.05k forks source link

Use argument-dependent-lookup for tuple_caster's get #5162

Open meghprkh opened 2 weeks ago

meghprkh commented 2 weeks ago

Description

Some libraries have custom container types and define their own get. They also extend the Pybind casters to bind their containers. This uses argument-dependent-lookup for using the correct "get" function

This allows me to use this as:

using mylib::get;
template <typename T1, typename T2>
class type_caster<mylib::pair<T1, T2>> : public tuple_caster<mylib::pair, T1, T2> {
};

Suggested changelog entry:

rwgk commented 2 weeks ago

FYI

I initiated Google global testing with this PR: it runs millions of tests including for a large number of 3rd-party packages imported from GitHub.

That's a quick (for me) way to answer the question: Does opening the ADL door have observable unfortunate side-effects? (Machines have to work hard for this, but that's ok.)

rwgk commented 2 weeks ago

I initiated Google global testing with this PR: it runs millions of tests including for a large number of 3rd-party packages imported from GitHub.

Global testing passed. :-) (For easy future reference, the internal testing ID is OCL:642715366:BASE:643022744:1718295770134:9c979c0d).

The change looks good to me. Could you please add tests, ideally covering all 4 changes? (I'm guessing you can just add tests in tests/test_pytypes.cpp/py)

meghprkh commented 2 weeks ago

Thanks for testing.

I had a discussion and it seems he better way to do this is composition of the std caster for now, since it does not bring mylib::get into global namespace. I am unsure how to do so, I might convert this PR to draft and look at it again in some while

But to explain my point something along the lines:

template <typename T1, typename T2>
class type_caster<mylib::pair<T1, T2>> : public tuple_caster<mylib::pair, T1, T2> {
using mylib::get;
};

// OR

template <typename T1, typename T2>
class type_caster<mylib::pair<T1, T2>> : public tuple_caster<mylib::get, mylib::pair, T1, T2> {
}; // cant really do as get is not a type and cant decltype it easily due to overloads

Currently I have shifted to doing something like:

template <typename T1, typename T2>
struct type_caster<mylib::pair<T1, T2>> {
    using mylib_pair_t = mylib::pair<T1, T2>;
    make_caster<std::pair<T1, T2>> std_pair_caster;

  public:
    PYBIND11_TYPE_CASTER(mylib_pair_t, _("mylib::pair<>"));

    bool load(handle src, bool convert)
    {
        if (!std_pair_caster.load(src, convert))
            return false;
        value = mylib::make_pair(std::get<0>(std::move(std_pair_caster.value)),
                               std::get<1>(std::move(std_pair_caster.value)));
        return true;
    }

    static handle cast(mylib_pair_t          src,
                       return_value_policy policy,
                       handle              parent)
    {
        auto std_pair = std::make_pair(mylib::get<0>(mylib::move(src)),
                                       mylib::get<1>(mylib::move(src)));
        return pybind11::cast(std_pair, policy, parent);
    }
};
rwgk commented 2 weeks ago

Quick tangential drive-by comments (in case other people somehow feel like copy-pasting from the code you posted):

PYBIND11_TYPE_CASTER(mylib_pair_t, _("mylib::pair<>"));

TBH I don't think I fully understand your situation (too busy). Will look again in case you get back to it with updates.