Add pybind11::bytearray by in42 · Pull Request #2799 · pybind/pybind11

in42 · 2021-01-16T11:10:21Z

Description

Implement #2523

Suggested changelog entry:

Add ``pybind11::bytearray`` to manipulate ``bytearray`` similar to ``bytes``.

YannickJadoul · 2021-01-16T13:30:40Z

Nice; thanks! This is still "[WIP]", I see, but just ping me whenever you're ready for review :-)

I guess we'd need a few more useful methods, and of course some tests (test_pytypes.{cpp,py} is probably where you want to add a few things ;-) )

in42 · 2021-01-18T02:06:06Z

Nice; thanks! This is still "[WIP]", I see, but just ping me whenever you're ready for review :-)

I guess we'd need a few more useful methods, and of course some tests (test_pytypes.{cpp,py} is probably where you want to add a few things ;-) )

Hi, I have added tests and few more methods. Can you review the patch? I am especially not sure if I have added the tests correctly or not.

in42 · 2021-01-18T02:12:17Z

Nice; thanks! This is still "[WIP]", I see, but just ping me whenever you're ready for review :-)

I guess we'd need a few more useful methods, and of course some tests (test_pytypes.{cpp,py} is probably where you want to add a few things ;-) )

Also do I need to add to the documentation?

bstaletic · 2021-01-19T15:00:41Z

+        char *buffer = PyByteArray_AsString(m_ptr);
+        return std::string(buffer);


Get the size as well, with PyByteArray_GET_SIZE, because std::string{buffer, size} is more efficient.

Do we need to free(buffer) after passing it off to the string?

Can PyByteArray_AsString() ever return NULL? Perhaps we need to guard against it.

Done

I don't think we need to free the buffer since PyByteArray_AsString is not allocating anything it is just returning a pointer to already allocated bytearray as far as I understand.

PyByteArray_AsString() will return NULL only if m_ptr which it will never be as it is checked in the constructor. So in fact I am now using macro versions of the methods as documented here: https://docs.python.org/3/c-api/bytearray.html which do not check if m_ptr is NULL.

YannickJadoul

Nice! Thanks for this :-)

Possibly useful functions (mostly stolen from https://docs.python.org/3/c-api/bytearray.html):

char *get() and/or operator char*()? I think this wouldn't copy, like string does, and would allow to change the contents? Maybe that's a reason to not want this, though.
size_t size(); this seems potentially useful.
bytesarray concat(bytesarray): PyByteArray_Concat exists. Not sure how useful it would be, though? Not crucial to have, afaic.
resize: PyByteArray_Resize exists.
Something to assign and change the contents of the bytearray? I thought that was the whole point of a bytearray, being a mutable bytes? @bstaletic, any thoughts?

YannickJadoul

(Whoops, sorry; didn't mean to approve yet. I definitely will, but there's a few more things to resolve.)

What a mess. Apologies, @in42.

Didn't mean to approve

bstaletic

char get() and/or operator char()? I think this wouldn't copy, like string does, and would allow to change the contents? Maybe that's a reason to not want this, though

data()? That would be in line with STL.

size_t size(); this seems potentially useful.

Agreed.

bytesarray concat(bytesarray): PyByteArray_Concat exists. Not sure how useful it would be, though? Not crucial to have, afaic.

operator+=()?

resize: PyByteArray_Resize exists.

Agreed.

Something to assign and change the contents of the bytearray? I thought that was the whole point of a bytearray, being a mutable bytes? @bstaletic, any thoughts?

operator[](ssize_t)?

With all the overloaded operators, it would actually feel like a C++ type.

YannickJadoul · 2021-01-22T00:55:32Z

With all the overloaded operators, it would actually feel like a C++ type.

That's true, yes! Of course, that's also up to @in42. I'm not saying this needs to per se go into this PR in order for this PR to be useful. So up to @in42 to judge what could still go in?

operator[](ssize_t)?

Hmm, there's no dedicated C API for this, is there? So this might already be supported (though slow, since there's no C API) through inheritance from object_api<T>?

bstaletic · 2021-01-22T08:13:21Z

Hmm, there's no dedicated C API for this, is there? So this might already be supported (though slow, since there's no C API) through inheritance from object_api?

I'd actually be surprised that the generic PyObject_GetAttrString() would work here. Also, operator[](ssize_t) could be accompanied with at(ssize_t), matching STL containers.

That's true, yes! Of course, that's also up to @in42. I'm not saying this needs to per se go into this PR in order for this PR to be useful. So up to @in42 to judge what could still go in?

Fair enough.

in42 · 2021-01-26T22:25:43Z

Nice! Thanks for this :-)

Possibly useful functions (mostly stolen from https://docs.python.org/3/c-api/bytearray.html):

char *get() and/or operator char*()? I think this wouldn't copy, like string does, and would allow to change the contents? Maybe that's a reason to not want this, though.

size_t size(); this seems potentially useful.

bytesarray concat(bytesarray): PyByteArray_Concat exists. Not sure how useful it would be, though? Not crucial to have, afaic.

resize: PyByteArray_Resize exists.

Something to assign and change the contents of the bytearray? I thought that was the whole point of a bytearray, being a mutable bytes? @bstaletic, any thoughts?

I will add the additional methods - but I remember seeing for pybind11::bytes, these methods were not defined. Is there any reason why they were not implemented for pybind11::bytes?

bstaletic · 2021-01-26T22:39:47Z

A few possible reasons:

Nobody requested it before. Simple omission.
It was postponed for "later".
bytes being immutable, it's worth taking a second look at whether CharT& operator[](size_t); is a good idea.

YannickJadoul · 2021-01-26T23:02:38Z

@in42, if you don't see a use for it, you don't have to ofc, as I suggested. But as far as I can tell, it could be useful. Especially so for bytearray, as it's a mutable structure.

YannickJadoul

One more implicit conversion remark. I'm reasonably confident @bstaletic will feel the same about this?

bstaletic · 2021-02-14T12:46:17Z

Final thought: commit history is a mess here. There are at least three empty commits and none of the 14 commits suggests that it has anything to do with py::bytearray.

in42 · 2021-02-14T12:54:54Z

Final thought: commit history is a mess here. There are at least three empty commits and none of the 14 commits suggests that it has anything to do with py::bytearray.

I am little unfamiliar with how changes are merged using a pull request. Aren't you supposed to squash the commits and merge them in a single commit? Otherwise I can rebase my changes in a single commit but the discussions will probably be lost then.

I am more familiar with the gerrit model.

YannickJadoul · 2021-02-14T12:58:24Z

@bstaletic, the pybind11 GitHub project will squash anyway, and use the title of this PR, I believe. So we should be fine?

bstaletic · 2021-02-14T13:03:10Z

Alright, if @YannickJadoul says not to worry about history, fine by me.

YannickJadoul · 2021-02-14T14:51:04Z

It's been long enough, and both @bstaletic and I approved. Thanks for holding on and making the PR, @in42! :-)

v2.7.0 (Jul 16, 2021) --------------------- New features: * Enable ``py::implicitly_convertible<py::none, ...>`` for ``py::class_``-wrapped types. `#3059 <https://github.com/pybind/pybind11/pull/3059>`_ * Allow function pointer extraction from overloaded functions. `#2944 <https://github.com/pybind/pybind11/pull/2944>`_ * NumPy: added ``.char_()`` to type which gives the NumPy public ``char`` result, which also distinguishes types by bit length (unlike ``.kind()``). `#2864 <https://github.com/pybind/pybind11/pull/2864>`_ * Add ``pybind11::bytearray`` to manipulate ``bytearray`` similar to ``bytes``. `#2799 <https://github.com/pybind/pybind11/pull/2799>`_ * ``pybind11/stl/filesystem.h`` registers a type caster that, on C++17/Python 3.6+, converts ``std::filesystem::path`` to ``pathlib.Path`` and any ``os.PathLike`` to ``std::filesystem::path``. `#2730 <https://github.com/pybind/pybind11/pull/2730>`_ * A ``PYBIND11_VERSION_HEX`` define was added, similar to ``PY_VERSION_HEX``. `#3120 <https://github.com/pybind/pybind11/pull/3120>`_ Signed-off-by: Zheng Ruoqin <zhengrq.fnst@fujitsu.com> Signed-off-by: Khem Raj <raj.khem@gmail.com>

crimsoncor · 2021-07-26T23:39:20Z

great feature. I'm actually making use of it today. But it isn't documented anywhere. I was able to go through the PR and the source to figure out how to use it, but should probably be added to the documentation

v2.7.0 (Jul 16, 2021) --------------------- New features: * Enable ``py::implicitly_convertible<py::none, ...>`` for ``py::class_``-wrapped types. `#3059 <https://github.com/pybind/pybind11/pull/3059>`_ * Allow function pointer extraction from overloaded functions. `#2944 <https://github.com/pybind/pybind11/pull/2944>`_ * NumPy: added ``.char_()`` to type which gives the NumPy public ``char`` result, which also distinguishes types by bit length (unlike ``.kind()``). `#2864 <https://github.com/pybind/pybind11/pull/2864>`_ * Add ``pybind11::bytearray`` to manipulate ``bytearray`` similar to ``bytes``. `#2799 <https://github.com/pybind/pybind11/pull/2799>`_ * ``pybind11/stl/filesystem.h`` registers a type caster that, on C++17/Python 3.6+, converts ``std::filesystem::path`` to ``pathlib.Path`` and any ``os.PathLike`` to ``std::filesystem::path``. `#2730 <https://github.com/pybind/pybind11/pull/2730>`_ * A ``PYBIND11_VERSION_HEX`` define was added, similar to ``PY_VERSION_HEX``. `#3120 <https://github.com/pybind/pybind11/pull/3120>`_ Signed-off-by: Zheng Ruoqin <zhengrq.fnst@fujitsu.com> Signed-off-by: Khem Raj <raj.khem@gmail.com>

v2.7.0 (Jul 16, 2021) --------------------- New features: * Enable ``py::implicitly_convertible<py::none, ...>`` for ``py::class_``-wrapped types. `#3059 <https://github.com/pybind/pybind11/pull/3059>`_ * Allow function pointer extraction from overloaded functions. `#2944 <https://github.com/pybind/pybind11/pull/2944>`_ * NumPy: added ``.char_()`` to type which gives the NumPy public ``char`` result, which also distinguishes types by bit length (unlike ``.kind()``). `#2864 <https://github.com/pybind/pybind11/pull/2864>`_ * Add ``pybind11::bytearray`` to manipulate ``bytearray`` similar to ``bytes``. `#2799 <https://github.com/pybind/pybind11/pull/2799>`_ * ``pybind11/stl/filesystem.h`` registers a type caster that, on C++17/Python 3.6+, converts ``std::filesystem::path`` to ``pathlib.Path`` and any ``os.PathLike`` to ``std::filesystem::path``. `#2730 <https://github.com/pybind/pybind11/pull/2730>`_ * A ``PYBIND11_VERSION_HEX`` define was added, similar to ``PY_VERSION_HEX``. `#3120 <https://github.com/pybind/pybind11/pull/3120>`_ Signed-off-by: Zheng Ruoqin <zhengrq.fnst@fujitsu.com> Signed-off-by: Khem Raj <raj.khem@gmail.com> Signed-off-by: Trevor Gamblin <trevor.gamblin@windriver.com>

gatopeich · 2021-11-12T07:27:25Z

Late to the party here, but it would be great to have an implicit conversion to string_view like bytes and str do.

Skylion007 · 2021-11-12T20:41:38Z

@gatopeich PR welcome on this front.

Add initial implementation

e9715aa

in42 changed the title ~~Add pybind11::bytearray~~ [WIP] Add pybind11::bytearray Jan 16, 2021

in42 added 3 commits January 17, 2021 14:59

Add few more methods

f607da7

Add tests

71cd50d

Fix a typo

1bf3fd3

in42 changed the title ~~[WIP] Add pybind11::bytearray~~ [RFC] Add pybind11::bytearray Jan 18, 2021

bstaletic reviewed Jan 19, 2021

View reviewed changes

in42 added 2 commits January 20, 2021 22:01

Use std::string constructor which takes size

0472a64

Fix implicit sign conversion error

2745cd9

YannickJadoul previously approved these changes Jan 20, 2021

View reviewed changes

Comment thread include/pybind11/pytypes.h Outdated

Comment thread include/pybind11/pytypes.h Outdated

Comment thread include/pybind11/pytypes.h

Comment thread include/pybind11/pytypes.h Outdated

YannickJadoul self-requested a review January 20, 2021 23:07

YannickJadoul reviewed Jan 20, 2021

View reviewed changes

YannickJadoul self-requested a review January 20, 2021 23:08

bstaletic reviewed Jan 21, 2021

View reviewed changes

Comment thread include/pybind11/pytypes.h

in42 changed the title ~~[RFC] Add pybind11::bytearray~~ [WIP] Add pybind11::bytearray Feb 5, 2021

in42 added 3 commits February 13, 2021 10:21

Add size method and test

95d348d

Remove implicit conversion

95ee903

Fix bytearray constructors and operator std::string()

39756e2

YannickJadoul added this to the v2.7 milestone Feb 13, 2021

YannickJadoul approved these changes Feb 13, 2021

View reviewed changes

Comment thread include/pybind11/pytypes.h Outdated

Make implicit bytearray constructor explicit

a0ea2d7

in42 added 2 commits February 14, 2021 10:22

Rerun tests

751a530

Rerun tests - 2

900ccdd

Remove NULL check

f95c937

YannickJadoul changed the title ~~[WIP] Add pybind11::bytearray~~ Add pybind11::bytearray Feb 14, 2021

YannickJadoul merged commit 417067e into pybind:master Feb 14, 2021

github-actions Bot added the needs changelog Possibly needs a changelog entry label Feb 14, 2021

YannickJadoul mentioned this pull request Feb 14, 2021

[FEAT] Wrap bytearray with pybind11::bytearray #2523

Closed

in42 deleted the bytearray branch February 14, 2021 17:33

henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021

gatopeich mentioned this pull request Nov 12, 2021

[Feature request] Support bytearray argument as string_view #2975

Open

porrashuang mentioned this pull request Feb 9, 2022

Cast bytearray to string #3707

Merged

fcladera mentioned this pull request Feb 9, 2023

Events type is bytes, not bytearray ros-event-camera/event_camera_py#2

Merged

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2799

Closed

		char *buffer = PyByteArray_AsString(m_ptr);
		return std::string(buffer);

Uh oh!

Conversation

in42 commented Jan 16, 2021 • edited by henryiii Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

YannickJadoul commented Jan 16, 2021

Uh oh!

in42 commented Jan 18, 2021

Uh oh!

in42 commented Jan 18, 2021

Uh oh!

bstaletic Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

in42 Jan 20, 2021

Choose a reason for hiding this comment

Uh oh!

YannickJadoul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

YannickJadoul left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YannickJadoul commented Jan 22, 2021

Uh oh!

bstaletic commented Jan 22, 2021

Uh oh!

in42 commented Jan 26, 2021

Uh oh!

bstaletic commented Jan 26, 2021

Uh oh!

YannickJadoul commented Jan 26, 2021

Uh oh!

YannickJadoul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bstaletic commented Feb 14, 2021

Uh oh!

in42 commented Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YannickJadoul commented Feb 14, 2021

Uh oh!

bstaletic commented Feb 14, 2021

Uh oh!

YannickJadoul commented Feb 14, 2021

Uh oh!

crimsoncor commented Jul 26, 2021

Uh oh!

gatopeich commented Nov 12, 2021

Uh oh!

Skylion007 commented Nov 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

in42 commented Jan 16, 2021 •

edited by henryiii

Loading

YannickJadoul left a comment •

edited

Loading

in42 commented Feb 14, 2021 •

edited

Loading