bpo-15913 : Add PyBuffer_SizeFromFormat() by nanjekyejoannah · Pull Request #13873 · python/cpython

nanjekyejoannah · 2019-06-06T20:50:31Z

Added implementation for PyBuffer_SizeFromFormat().

https://bugs.python.org/issue15913

nanjekyejoannah · 2019-06-06T21:45:30Z

pablogsal · 2019-06-06T22:48:00Z

I think this needs tests. Implementation aside, calling the current function with 3si 3s 0i segfaults for me with a bus error.

nanjekyejoannah · 2019-06-06T23:03:02Z

@pablogsal , thought about that also but am still wondering where to place this test?

pablogsal · 2019-06-06T23:07:19Z

@pablogsal , thought about that also but am still wondering where to place this test?

I would say exposed in Modules/_testcapimodule.c and then tested with Lib/test/test_capi.py but Victor or Antoine would know better regarding this specific API.

Doc/c-api/buffer.rst

Objects/abstract.c

Doc/c-api/buffer.rst

bedevere-bot · 2019-06-07T16:48:53Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2019-06-07T16:49:55Z

Would it be possible to add an unit test? Expose the function in _testcapi and add a test in test_buffer using _testcapi (such test should be marked with @cpython_only, or skipped if _testcapi is None).

nanjekyejoannah · 2019-06-07T16:58:10Z

I have made some changes locally that will address some of the comments @vstinner. I will update the PR when I finish the tests.

vstinner · 2019-06-07T17:21:21Z

I have made some changes locally that will address some of the comments @vstinner. I will update the PR when I finish the tests.

Ok. Ping me when I should review again ;-)

Lib/test/test_buffer.py

Objects/abstract.c

Lib/test/test_buffer.py

Objects/abstract.c

Doc/c-api/buffer.rst

ZackerySpytz · 2019-06-24T14:47:49Z

Modules/_testcapimodule.c

+        return NULL;
+    }
+
+    return PyLong_FromSsize_t(PyBuffer_SizeFromFormat(format));


I don't believe PyBuffer_SizeFromFormat() should be inlined like this. Please check it for failure before the PyLong_FromSsize_t() call.

Oh right, NULL should be returned if the result is -1.

ZackerySpytz · 2019-06-24T14:48:33Z

Objects/abstract.c

+    }
+
+    itemsize = PyLong_AsSsize_t(res);
+    if (itemsize < 0) {


This check is not needed.

calcsize() might return a integer which doesn't fit into a C Py_ssize_t. It's unlikely, but I prefer to keep the test.

I know that. I meant that the explicit check is not needed because the control flow will fall through to the done label.

vstinner · 2019-06-24T16:13:41Z

Without .encode, the test failed with:

-----------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python/cpython/Lib/test/test_buffer.py", line 4425, in test_pybuffer_size_from_format
    struct.calcsize(format))
BytesWarning: Comparison between bytes and string

You should be able to reproduce rhe issue using: ./python -bb -m tets test_buffer

The test should pass without .encode. You should investigate why it fails. struct.calcsize() accepts format as a Unicode string, no?

nanjekyejoannah · 2019-08-16T16:59:50Z

The test should pass without .encode. You should investigate why it fails. struct.calcsize() accepts >format as a Unicode string, no?

I needed to change fmt = PyBytes_FromString(format) to fmt = PyUnicode_FromString(format); in order to do away with the comparison between bytes and string error.

The test passes now :

gitpod /workspace/cpython $ ./python -bb -m test test_buffer
Run tests sequentially
0:00:00 load avg: 0.69 [1/1] test_buffer

== Tests result: SUCCESS ==

1 test OK.

nanjekyejoannah · 2019-08-16T17:28:04Z

I have made the requested changes; please review again .

bedevere-bot · 2019-08-16T17:28:08Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner · 2019-08-19T11:29:50Z

Misc/NEWS.d/next/Core and Builtins/2019-06-06-20-52-38.bpo-15913.5Sg5cv.rst

@@ -0,0 +1 @@
+:c:func:`PyBuffer_SizeFromFormat()` now added.


I suggest:

Implement :c:func:`PyBuffer_SizeFromFormat()` function (previously documented but not implemented): call :func:`struct.calcsize`.

vstinner · 2019-08-19T11:31:38Z

Lib/test/test_buffer.py


+    @support.cpython_only
+    def test_pybuffer_size_from_format(self):
+        nitems = randrange(1, 30)


I would prefer to avoid using random in such test. iter_format() returns 91 items: it sounds overkill to me. IMHO tTesting 3 formats should but enough, like:

for fmt in ('', 'ii', '3s'):

vstinner · 2019-08-19T11:32:55Z

Modules/_testcapimodule.c

    {"test_pep3118_obsolete_write_locks", (PyCFunction)test_pep3118_obsolete_write_locks, METH_NOARGS},
 #endif
    {"getbuffer_with_null_view", getbuffer_with_null_view, METH_O},
+    {"pybuffer_size_from_format", (PyCFunction)pybuffer_size_from_format, METH_VARARGS},


Maybe reuse the same name:

Suggested change

{"pybuffer_size_from_format", (PyCFunction)pybuffer_size_from_format, METH_VARARGS},

{"PyBuffer_SizeFromFormat", test_PyBuffer_SizeFromFormat, METH_VARARGS},

Note: I don't think that (PyCFunction) cast is needed here.

bedevere-bot · 2019-08-19T11:33:05Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

vstinner · 2019-08-19T13:20:56Z

Modules/_testcapimodule.c

+    const char *format;
+    Py_ssize_t result;
+
+    if (!PyArg_ParseTuple(args, "s:pybuffer_size_from_format",


You forget to update the function name here.

vstinner · 2019-08-19T13:21:13Z

Lib/test/test_buffer.py

+    @support.cpython_only
+    def test_pybuffer_size_from_format(self):
+        # basic tests
+        for format in ("i", "3s", "0i", " "):


" " <= my proposal was to test an empty format string

nanjekyejoannah · 2019-08-19T15:41:55Z

@vstinner I don't think this CI fail is now my business. Is it? i.e

Windows PR Tests win64: "The job running on agent Azure Pipelines 7 has exceeded the maximum execution time of 60. For more information, see https://go.microsoft.com/fwlink/?linkid=2077134 "

vstinner · 2019-08-20T14:47:09Z

I merged your PR @nanjekyejoannah, thanks!

@vstinner I don't think this CI fail is now my business.

I don't know why Azure CI failed, but this CI is not mandatory yet.

Implement PyBuffer_SizeFromFormat() function (previously documented but not implemented): call struct.calcsize().

Add PyBuffer_SizeFromFormat

3bf3ff7

the-knights-who-say-ni added the CLA signed label Jun 6, 2019

bedevere-bot added the awaiting review label Jun 6, 2019

blurb-it bot and others added 3 commits June 6, 2019 20:52

📜🤖 Added by blurb_it.

74dd813

Mark version

caa5b68

remove white space

aef0745

nanjekyejoannah changed the title ~~bpo-15913 : PyBuffer_SizeFromFormat is missing~~ bpo-15913 : Add PyBuffer_SizeFromFormat() Jun 6, 2019

pablogsal requested a review from vstinner June 6, 2019 22:03

nanjekyejoannah changed the title ~~bpo-15913 : Add PyBuffer_SizeFromFormat()~~ [WIP]bpo-15913 : Add PyBuffer_SizeFromFormat() Jun 6, 2019

nanjekyejoannah changed the title ~~[WIP]bpo-15913 : Add PyBuffer_SizeFromFormat()~~ bpo-15913 : Add PyBuffer_SizeFromFormat() Jun 6, 2019

nanjekyejoannah changed the title ~~bpo-15913 : Add PyBuffer_SizeFromFormat()~~ [WIP]bpo-15913 : Add PyBuffer_SizeFromFormat() Jun 7, 2019

vstinner requested changes Jun 7, 2019

View reviewed changes

Doc/c-api/buffer.rst Outdated Show resolved Hide resolved

Objects/abstract.c Show resolved Hide resolved

Objects/abstract.c Outdated Show resolved Hide resolved

Doc/c-api/buffer.rst Show resolved Hide resolved

bedevere-bot removed the awaiting review label Jun 7, 2019

bedevere-bot added the awaiting changes label Jun 7, 2019

Improve draft PR

30ac72a

nanjekyejoannah changed the title ~~[WIP]bpo-15913 : Add PyBuffer_SizeFromFormat()~~ bpo-15913 : Add PyBuffer_SizeFromFormat() Jun 11, 2019

fix test

31442ab

vstinner requested changes Jun 11, 2019

View reviewed changes

Lib/test/test_buffer.py Outdated Show resolved Hide resolved

Objects/abstract.c Outdated Show resolved Hide resolved

Objects/abstract.c Outdated Show resolved Hide resolved

Objects/abstract.c Outdated Show resolved Hide resolved

Objects/abstract.c Outdated Show resolved Hide resolved

pablogsal reviewed Jun 11, 2019

View reviewed changes

Lib/test/test_buffer.py Outdated Show resolved Hide resolved

pablogsal reviewed Jun 11, 2019

View reviewed changes

Objects/abstract.c Outdated Show resolved Hide resolved

Fix tests, doc and logic

98a5576

vstinner reviewed Jun 15, 2019

View reviewed changes

Objects/abstract.c Outdated Show resolved Hide resolved

Doc/c-api/buffer.rst Outdated Show resolved Hide resolved

ZackerySpytz reviewed Jun 24, 2019

View reviewed changes

nanjekyejoannah and others added 2 commits June 24, 2019 12:09

Revert encode()

2cbcec3

Fix segfault

1dd3c2a

Fix bytes and string comparison error

5cac74f

bedevere-bot added awaiting change review and removed awaiting merge labels Aug 16, 2019

vstinner requested changes Aug 19, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Aug 19, 2019

nanjekyejoannah and others added 2 commits August 19, 2019 12:50

rename function and update news entry

eddee93

update news entry

244fdce

vstinner reviewed Aug 19, 2019

View reviewed changes

nanjekyejoannah added 2 commits August 19, 2019 13:21

revert deleted line

dda73c3

forgotten rename and test update

f3e8c51

vstinner merged commit 9e66aba into python:master Aug 20, 2019

bedevere-bot removed the awaiting changes label Aug 20, 2019

nanjekyejoannah deleted the issue15913 branch September 5, 2019 22:11

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019

bpo-15913: Implement PyBuffer_SizeFromFormat() (pythonGH-13873)

5397af3

Implement PyBuffer_SizeFromFormat() function (previously documented but not implemented): call struct.calcsize().

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020

bpo-15913: Implement PyBuffer_SizeFromFormat() (pythonGH-13873)

89d1953

Implement PyBuffer_SizeFromFormat() function (previously documented but not implemented): call struct.calcsize().

websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020

bpo-15913: Implement PyBuffer_SizeFromFormat() (pythonGH-13873)

14a49d1

Implement PyBuffer_SizeFromFormat() function (previously documented but not implemented): call struct.calcsize().

		@@ -0,0 +1 @@
		:c:func:`PyBuffer_SizeFromFormat()` now added. No newline at end of file

	{"pybuffer_size_from_format", (PyCFunction)pybuffer_size_from_format, METH_VARARGS},
	{"PyBuffer_SizeFromFormat", test_PyBuffer_SizeFromFormat, METH_VARARGS},

Uh oh!

Conversation

nanjekyejoannah commented Jun 6, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanjekyejoannah commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanjekyejoannah commented Jun 6, 2019

Uh oh!

pablogsal commented Jun 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Jun 7, 2019

Uh oh!

vstinner commented Jun 7, 2019

Uh oh!

nanjekyejoannah commented Jun 7, 2019

Uh oh!

vstinner commented Jun 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jun 24, 2019

Uh oh!

nanjekyejoannah commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanjekyejoannah commented Aug 16, 2019

Uh oh!

bedevere-bot commented Aug 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nanjekyejoannah commented Aug 19, 2019

Uh oh!

vstinner commented Aug 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nanjekyejoannah commented Jun 6, 2019 •

edited by bedevere-bot

Loading

nanjekyejoannah commented Jun 6, 2019 •

edited

Loading

pablogsal commented Jun 6, 2019 •

edited

Loading

nanjekyejoannah commented Aug 16, 2019 •

edited

Loading