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

cast pointer to std::tuple and std::pair #2334

Merged
merged 1 commit into from
Jul 28, 2020

Conversation

wojdyr
Copy link
Contributor

@wojdyr wojdyr commented Jul 27, 2020

This PR copies handle cast(T *src, ...) from PYBIND11_TYPE_CASTER to tuple_caster.
It enables bindings to functions returning a pointer to std::tuple or std::pair - in the same way as pybind11 handles now pointers to int, std::string, std::array, and everything else.

I don't have a deep understanding of pybind11 internals - please double check this patch before applying.

@YannickJadoul
Copy link
Collaborator

I'm still a little bit in doubt on whether we actually want this. After all, std::optional<std::tuple<...>> is just as easy to write, no?

On the other hand, consistency with the other casters might not be bad...

@wojdyr
Copy link
Contributor Author

wojdyr commented Jul 27, 2020

If the existing C++ code returns a pointer, converting it to std::optional in the bindings is more involved (and requires C++17) than just writing m.def("func", &func)

@bstaletic
Copy link
Collaborator

I'm still a little bit in doubt on whether we actually want this.

On the account of "principle of least surprise" (alternatively WTF/s), I think we do want this. Currently these casters seem inconsistent with the rest.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Jul 27, 2020

Fun plot twist. This also doesn't compile:

m.def("f", [](std::pair<int, double> *p){ std::cout << p->first << ", " << p->second << std::endl; });

So does it make sense to allow returning std::pair<T, U>*/std::tuple<Ts...>* when you can't take them as arguments?

Apart from that, I would agree that this should be more similar, yes.

@YannickJadoul
Copy link
Collaborator

YannickJadoul commented Jul 27, 2020

So does it make sense to allow returning std::pair<T, U>*/std::tuple<Ts...>* when you can't take them as arguments?

Actually. Let me answer my own question:

No, it doesn't. But it's already like that for the other types as well, so there's no inconsistency between different casters.

EDIT: I meant you can accept pointers for the other casters, but None doesn't become a nullptr. Not being able to use std::pair<T, U>* as argument type is still a difference from e.g. std::string or std::vector.

@wojdyr
Copy link
Contributor Author

wojdyr commented Jul 27, 2020

I implemented only the C++ -> Python conversion, because I don't know how to do the Python -> C++ part.
Someone smarter is needed for this.

@YannickJadoul
Copy link
Collaborator

I implemented only the C++ -> Python conversion, because I don't know how to do the Python -> C++ part.
Someone smarter is needed for this.

We're struggling ourselves with that: see e.g. #2245

Can I ask: since you want to be able to return pointers to things that will get copied anyway. How do you see accepting non-const references and pointers to std::pair, std::tuple, std::string, std::vector (under stl.h), ... (i.e., anything that is copied and wouldn't really be a reference/pointer to the Python object) ? Is that necessary to support?
We were just seriously thinking of ditching that, to solve #2245.

@wojdyr
Copy link
Contributor Author

wojdyr commented Jul 28, 2020

@YannickJadoul I think it's useful to get automatic bindings even in case of non-const references or pointers, but I'm not the best person to discuss it.

@YannickJadoul
Copy link
Collaborator

@YannickJadoul I think it's useful to get automatic bindings even in case of non-const references or pointers, but I'm not the best person to discuss it.

Thanks; any bit of input is interesting. And I see your concern.
The thing why we're considering this is because it would solve an issue and that these references and pointers are misleading (because it's a reference/pointer to a temporary, copied object).

Anyhow, thanks! :-)

Copy link
Collaborator

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then yes, why not get this in. At least it's a bit more consistency, until other caster issues get fixed!

@wjakob
Copy link
Member

wjakob commented Jul 28, 2020

Does the following shorter approach not work?

    static handle cast(T *src, return_value_policy policy, handle parent) {
        if (!src) return none().release();
        return cast(*src, policy, parent);
    }

@wjakob
Copy link
Member

wjakob commented Jul 28, 2020

Ooops, scratch that previous comment. LGTM.

@YannickJadoul YannickJadoul merged commit 8e40e38 into pybind:master Jul 28, 2020
@YannickJadoul
Copy link
Collaborator

Thanks, @wojdyr! :-)

@YannickJadoul
Copy link
Collaborator

(Quickly still cross-referencing to #1666, which was already closed)

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.

4 participants