Skip to content

/cpython

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

bpo-39200: Correct the error message for range() empty constructor #17813

Merged
merged 4 commits into from Jan 5, 2020
Merged

bpo-39200: Correct the error message for range() empty constructor #17813

merged 4 commits into from Jan 5, 2020
+59 −29

Conversation

@pablogsal
Copy link
Member

pablogsal commented Jan 3, 2020 •
edited by bedevere-bot

https://bugs.python.org/issue39200
@pablogsal
bpo-39200: Correct the error message for range() empty constructor
Loading status checks…
7a5723d
@pablogsal pablogsal force-pushed the pablogsal:bpo-39200 branch from 3b628b2 to 7a5723d Jan 3, 2020
@corona10
corona10 approved these changes Jan 3, 2020
View changes
Copy link
Member

corona10 left a comment

For me, this is more correct :)
@tirkarthi tirkarthi mentioned this pull request Jan 3, 2020
Open
@serhiy-storchaka
serhiy-storchaka reviewed Jan 3, 2020
View changes
Copy link
Member

serhiy-storchaka left a comment

Would be nice to add tests.
Objects/rangeobject.c Outdated Show resolved Hide resolved
@serhiy-storchaka

This comment has been minimized.

Sign in to view
Copy link
Member

serhiy-storchaka commented Jan 3, 2020

Other option - just use PyArg_UnpackTuple(args, "range", 1, 3, &start, &stop, &step) and check if stop == NULL.
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member Author

pablogsal commented Jan 3, 2020 •
edited

Other option - just use PyArg_UnpackTuple(args, "range", 1, 3, &start, &stop, &step) and check if stop == NULL.

I have implemented the switch statement as we would need to do the dance with step and start anyway and I find the switch statement a bit more clear.
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member Author

pablogsal commented Jan 3, 2020 •
edited

Would be nice to add tests.

Done in eb9e784
@pablogsal pablogsal force-pushed the pablogsal:bpo-39200 branch 2 times, most recently from e73eab4 to eb9e784 Jan 3, 2020
@pablogsal

This comment has been minimized.

Sign in to view
Copy link
Member Author

pablogsal commented Jan 3, 2020 •
edited

Btw, as a bonus the constructor of range is anecdotally faster:

master

./python.exe -m pyperf timeit "range(1,3,4)"
.....................
Mean +- std dev: 893 ns +- 44 ns

patched

./python.exe -m pyperf timeit "range(1,3,4)"
.....................
Mean +- std dev: 822 ns +- 14 ns
@corona10
corona10 reviewed Jan 4, 2020
View changes
Copy link
Member

corona10 left a comment

@pablogsal I left comments on test code :)
Lib/test/test_range.py Outdated Show resolved Hide resolved
Lib/test/test_range.py Outdated Show resolved Hide resolved
@pablogsal
Get rid of PyArg_UnpackTuple
Verified
This commit was signed with a verified signature.
pablogsal Pablo Galindo
GPG key ID: FFE87404168BD847 Learn about signing commits
Loading status checks…
f8c70ad
@pablogsal pablogsal force-pushed the pablogsal:bpo-39200 branch from eb9e784 to f8c70ad Jan 4, 2020
@corona10
corona10 approved these changes Jan 4, 2020
View changes
@serhiy-storchaka
serhiy-storchaka reviewed Jan 4, 2020
View changes
Objects/rangeobject.c Outdated Show resolved Hide resolved
@pablogsal @serhiy-storchaka
Update Objects/rangeobject.c
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c226b4d 
Co-Authored-By: Serhiy Storchaka <storchaka@gmail.com>
@serhiy-storchaka
serhiy-storchaka approved these changes Jan 4, 2020
View changes
@ZackerySpytz
ZackerySpytz reviewed Jan 4, 2020
View changes
Lib/test/test_range.py Outdated Show resolved Hide resolved
Lib/test/test_range.py Outdated Show resolved Hide resolved
Objects/rangeobject.c Outdated Show resolved Hide resolved
Objects/rangeobject.c Outdated Show resolved Hide resolved
Objects/rangeobject.c Outdated Show resolved Hide resolved
@pablogsal
fixup! Update Objects/rangeobject.c
Verified
This commit was signed with a verified signature.
pablogsal Pablo Galindo
GPG key ID: FFE87404168BD847 Learn about signing commits
Loading status checks…
d70a48f
@serhiy-storchaka
serhiy-storchaka approved these changes Jan 5, 2020
View changes
@pablogsal pablogsal merged commit 4b66fa6 into python:master Jan 5, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200104.13 succeeded
Details
bedevere/issue-number Issue number 39200 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pablogsal pablogsal deleted the pablogsal:bpo-39200 branch Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZackerySpytz ZackerySpytz

@serhiy-storchaka serhiy-storchaka

@corona10 corona10

Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@pablogsal @serhiy-storchaka @corona10 @ZackerySpytz @the-knights-who-say-ni @bedevere-bot
You can’t perform that action at this time.