-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Accessing C++ hash functions from Python and vice versa #1034
Conversation
There are two separate additions: 1. `py::hash(obj)` is equivalent to the Python `hash(obj)`. 2. `.def(hash(py::self))` registers the hash function defined by std::hash<T> as the Python hash function.
I think it would also be nice to provide a convenient way to mark a wrapped class as unhashable, but I'm not sure what a good syntax for that would be. |
include/pybind11/pytypes.h
Outdated
@@ -390,6 +390,13 @@ inline void setattr(handle obj, handle name, handle value) { | |||
inline void setattr(handle obj, const char *name, handle value) { | |||
if (PyObject_SetAttrString(obj.ptr(), name, value.ptr()) != 0) { throw error_already_set(); } | |||
} | |||
|
|||
inline long hash(object obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle
is generally used for function parameters to avoid an extra incref/decref of copying an object
.
Py_hash_t
is needed on Python 3 to resolve the compiler warnings, but it's not available on Python 2. Looking at the docs, ssize_t
should satisfy the return type of both 2 and 3:
Changed in version 3.2: The return type is now Py_hash_t. This is a signed integer the same size as Py_ssize_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Not sure how that ended up being object
- it was handle
in my brain. I decided to deal with the return type by letting decltype
handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the return type: It would be nice for pybind11's API to have the same type for both Python 2 and 3, so that we don't pass on these inconsistencies to users (unless it's absolutely necessary). If ssize_t
is compatible with both, it would be preferable IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think anything in the standard that prevents long
from being wider than Py_ssize_t
, but I don't know of any contemporary systems where that happens (it would probably have been true on 16-bit systems). I've switched to Py_ssize_t and added a static_assert
just to be safe.
tests/test_pytypes.py
Outdated
@@ -5,6 +5,18 @@ | |||
from pybind11_tests import debug_enabled | |||
|
|||
|
|||
class Hashable(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two test classes are only used in test_hash()
. Move them into the scope of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Something along the lines of |
That's certainly one option, but I didn't have a good what what to name the tag variable. Other options would be something like |
Could you also add a changelog entry for this? Regarding the |
Done. Not sure where in the changelog you feel this belongs, so feel free to directly commit to my branch to move it around.
Maybe something like |
Things can get a little bit tricky here because of the distinction between class_<Foo>(m, "Foo")
.attrs("__hash__"_a=py::none(),
"bar"_a=1,
"baz"_a="two")
.def("method", [](){}); To mirror the usual Python syntax: class Foo:
__hash__ = None
bar = 1
baz = "two"
def method():
pass |
Yes, I was also a bit worried about that, and thinking it might be a bit of a pain to implement. For me it wasn't totally obvious that setting .attrs("__hash__"_a=py::none(),
"bar"_a=1,
"baz"_a="two") I think I would rather have this as three separate function calls, each taking a name and a value (which I'm guessing would also simplify the implementation since it wouldn't need to muck about with variadic templates). Maybe it's worth adding a two-argument form of |
I've merged this now and the
Seems like it's mostly just a convenience for the C API, since it assigns
Sure, I can see that being preferable. But I'm not sure about allowing chaining for all |
There are two separate additions:
py::hash(obj)
is equivalent to the Pythonhash(obj)
..def(hash(py::self))
registers the hash function defined by std::hash as the Python hash function.