Simplify buffer generation in cybuffer
#11
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakirkham
force-pushed
the
skip_mk_memoryview
branch
from
fd16379
to
06ef25e
Compare
jakirkham
changed the title
memoryview from objcybuffer
jakirkham
force-pushed
the
skip_mk_memoryview
branch
4 times, most recently
from
543d368
to
719591b
Compare
Instead of relying on the `data` object passed into `__cinit__`, use `self.obj`, which points to the `data` object provided. That way if we decide to overwrite the `data` object, it shouldn't cause us any issues.
At this stage, there is no real value to creating a `memoryview` from our data to create an object vs. just using the buffer protocol to fill out our buffer. Under the hood, the `PyMemoryView_FromObject` call is doing exactly what we are doing and providing little more. Filling out the buffer directly avoids creating an intermediate object between our buffer and the data. Not to mention this should make it easier for us to get access to the underlying object used to create the buffer. Also should cutdown on the overhead of generating a `cybuffer` object.
Skip checking to see if the object supports the (new) buffer protocol unless we are on Python 2. On Python 2/3, `PyObject_GetBuffer` already raises a `TypeError` with an informative message. This is also true of `PyBuffer_FromObject` on Python 2. So it is better to allow Python's `TypeError` messages to propagate up instead of adding our own. This keeps the code simple for our use case with the same end effect. Also makes the Python 3 case a little cleaner. Of course on Python 2 if the object doesn't support (new) buffer protocol, it's good to figure that upfront as the check is cheap compared to catching an exception. That way we can fallback to the buffer protocol simply. Instead of carrying around a new object for storing the result, just overwrite `data` as we already have a copy of it earlier and this won't affect anything outside of this function. Any exceptions that would be propagated on the Python 2 side are handled or not just as before.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Drops generation of a
memoryviewfrom our data. UsingPyObject_GetBufferis already sufficient for our needs and does essentially the same work. Helps us avoid an unneeded intermediate step and container. Plus it simplifies the code a bit.Also only check to see if our data does not support the (new) buffer protocol on Python 2 for the purpose of trying to coerce it through the old buffer protocol. In all other cases, Python checks that the (new) buffer protocol is supported when calling
PyObject_GetBuffer. If the (new) buffer protocol is not supported, it will raise an exception anyways. So there is no need for us to do this.