Skip to content

bpo-37207: Use vectorcall for range() - #18464

Merged
miss-islington merged 5 commits into
python:masterfrom
encukou:vectorcall-newrange
Feb 18, 2020
Merged

bpo-37207: Use vectorcall for range()#18464
miss-islington merged 5 commits into
python:masterfrom
encukou:vectorcall-newrange

Conversation

@encukou

@encukouencukou commented Feb 11, 2020

Copy link
Copy Markdown
Member

This continues the range() part of #13930. The complete pull request is stalled on discussions around dicts, but range() should not be controversial. (And I plan to open PRs for other parts if this is merged.)
On top of Mark's change, I unified range_new and range_vectorcall, which had a lot of duplicate code.

https://bugs.python.org/issue37207

Automerge-Triggered-By: @encukou

Comment threadObjects/rangeobject.c Outdated
range_vectorcall(PyTypeObject *type, PyObject *const *args,
size_t nargsf, PyObject *kwnames)
{
size_t nargs = PyVectorcall_NARGS(nargsf);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I didn't expect a size_t here. I opened an issue to discuss PyVectorcall_NARGS() return type: https://bugs.python.org/issue39611

@encukouencukouFeb 18, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The return type already is Py_ssize_t. The bug was only in this patch.
(When Mark was originally writing the patch, there was a discussion of Py_ssize_t vs. size_t. Mark is a big proponent of the unsigned type, which is more correct. But also much less compatible with the existing codebase.)

Comment threadObjects/rangeobject.c Outdated
*/
static PyObject *
range_new(PyTypeObject *type, PyObject *args, PyObject *kw)
range_from_array(PyTypeObject *type, PyObject *const *args, size_t num_args)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer to use Py_ssize_t type for num_args. Usually, it's called "nargs", but I'm fine with "num_args" ;-)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Right, Py_ssize_t is more consistent.
But I find num_args more descriptive, so I'll keep that name.

@codecov

codecovBot commented Feb 11, 2020

Copy link
Copy Markdown

Codecov Report

Merging #18464 into master will increase coverage by 1.08%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #18464 +/- ##
===========================================
+ Coverage 82.11% 83.20% +1.08% 
===========================================
Files 1955 1571 -384 Lines 588958 414749 -174209 Branches 44428 44456 +28 ===========================================
- Hits 483632 345077 -138555 + Misses 95677 60024 -35653 + Partials 9649 9648 -1 
Impacted FilesCoverage Δ
Lib/distutils/tests/test_bdist_rpm.py30.00% <0.00%> (-65.00%)⬇️
Lib/distutils/command/bdist_rpm.py7.63% <0.00%> (-56.88%)⬇️
Lib/test/test_urllib2net.py76.92% <0.00%> (-13.85%)⬇️
Lib/test/test_smtpnet.py78.57% <0.00%> (-7.15%)⬇️
Lib/ftplib.py63.85% <0.00%> (-6.06%)⬇️
Lib/test/test_ftplib.py87.11% <0.00%> (-4.72%)⬇️
Tools/scripts/db2pickle.py17.82% <0.00%> (-3.97%)⬇️
Tools/scripts/pickle2db.py16.98% <0.00%> (-3.78%)⬇️
Lib/test/test_socket.py71.94% <0.00%> (-3.77%)⬇️
Lib/test/test_asyncio/test_base_events.py91.84% <0.00%> (-3.30%)⬇️
... and 465 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3e7ea5...b4559ce. Read the comment docs.

Comment threadObjects/rangeobject.c
PyErr_Format(PyExc_TypeError,
"range expected at most 3 arguments, got %zd",
"range expected at most 3 arguments, got %zu",
num_args);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be reset to %zd, since num_args is signed, no?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

You're right.

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

Sign up for freeto 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.

6 participants

@encukou@vstinner@the-knights-who-say-ni@bedevere-bot@miss-islington@markshannon