Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

meghprkh
Copy link

@meghprkh meghprkh commented Jun 12, 2024

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:

meghprkh and others added 3 commits June 12, 2024 15:01
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
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jun 12, 2024
@rwgk
Copy link
Collaborator

rwgk commented Jun 12, 2024

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
Copy link
Collaborator

rwgk commented Jun 13, 2024

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
Copy link
Author

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);
    }
};

@meghprkh meghprkh marked this pull request as draft June 13, 2024 19:19
@rwgk
Copy link
Collaborator

rwgk commented Jun 14, 2024

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<>"));
  • Use const_name, do not use _

  • "mylib::pair<>" isn't anything any type checker could recognize: specify actual Python types instead.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants