Skip to content

Support pointer-to-member-function in def_buffer #858

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

Merged
merged 1 commit into from
May 22, 2017

Conversation

bmerry
Copy link
Contributor

@bmerry bmerry commented May 17, 2017

Closes #857. Instead of calling the function object directly, it wraps
it in std::bind and then immediately calls that. This is a poor-man's
form of std::invoke (not available in C++11 or C++14).

@dean0x7d
Copy link
Member

The inclusion of <functional> in the main pybind11.h header may be a compilation time issue. At least I assume so, given that pybind11/functional.h is an optional extra and not part of the core headers. cc @wjakob

@jagerman
Copy link
Member

The use of <functional> is probably a non-starter (not including the rather heavy-weight <functional> in core pybind was a design decision from way back).

An overload that accepts a member function pointer and converts it into lambda, on the other hand, could work, and shouldn't be noticeably more code.

Also, tests_buffers seems like a better place than test_issues. The latter is quasi-deprecated, with a vague long-term plan to move everything out of it into more appropriate places, and to at least not add anything more into it.

@bmerry
Copy link
Contributor Author

bmerry commented May 18, 2017

Ok, I'll give it another go. I guess the simplest thing then would be to provide an overload of def_buffer that takes any PTMF, wraps it in a lambda, and passes it to the original overload?

@bmerry bmerry changed the title Support any kind of invocable in def_buffer Support pointer-to-member-function in def_buffer May 18, 2017
@bmerry
Copy link
Contributor Author

bmerry commented May 18, 2017

I've reworked it and changed the PR title to reflect the new version. I haven't flattened the history at this point.

Do you think it is also worth adding an overload for pointer to data member (so that the class can initialise a buffer_info member when constructed and then we just pass a pointer to that member to def_buffer?

@bmerry
Copy link
Contributor Author

bmerry commented May 18, 2017

The pypy build is crashing. Is that expected due to https://bitbucket.org/pypy/pypy/issues/2444 (the other to_python test in the file is disabled because of that, but I don't want to do the same here until someone can confirm that I'm not just hiding a real bug, particularly since that pypy bug seems into indicate a leak rather than a double-free.

Copy link
Member

@dean0x7d dean0x7d left a comment

Choose a reason for hiding this comment

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

Should be OK to skip the test on PyPy. Looks like the same issue as the existing test. The rest looks good to me. I've just noted a minor brace style issue.

@@ -74,6 +74,34 @@ class Matrix {
float *m_data;
};

struct PTMFBuffer
{
Copy link
Member

Choose a reason for hiding this comment

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

Brace style: same line.

};

class ConstPTMFBuffer
{
Copy link
Member

Choose a reason for hiding this comment

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

Same.

@bmerry
Copy link
Contributor Author

bmerry commented May 22, 2017

I've just noted a minor brace style issue.

Fixed, thanks.

@jagerman
Copy link
Member

Looks good to me.

@jagerman
Copy link
Member

Can you squash the changes and update the commit messages into a single commit?

@bmerry
Copy link
Contributor Author

bmerry commented May 22, 2017

Sure. Should I do that as a force-push to the branch?

@jagerman
Copy link
Member

Yup.

Closes pybind#857, by adding overloads to def_buffer that match pointers to
member functions and wrap them in lambdas.
@bmerry
Copy link
Contributor Author

bmerry commented May 22, 2017

Done, and also added the marker to disable the new test in PyPy. Once Travis and Appveyor are happy I think it should be good to go.

@jagerman
Copy link
Member

Looks like the last appveyor test got stuck, but all the rest passed so this is good to go.

@jagerman jagerman merged commit fe0cf8b into pybind:master May 22, 2017
@bmerry bmerry deleted the buffer-ptmf branch May 23, 2017 17:50
@dean0x7d dean0x7d modified the milestone: v2.2 Aug 13, 2017
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.

3 participants