Skip to content

bpo-42222: Modernize integer test/conversion in randrange() #23064

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.

Sign up for GitHub

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

Jump to bottom
Merged
rhettinger merged 9 commits into from
Dec 28, 2020
Merged

bpo-42222: Modernize integer test/conversion in randrange() #23064

rhettinger merged 9 commits into from
Dec 28, 2020

Conversation

@rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Oct 31, 2020 •
edited by bedevere-bot
Loading

Moves the int(x) == x test and conversion into the C code for operator.index().

Provides a 7% speed up:

$ pytime -s 'from random import randrange' 'randrange(15)'  # baseline
500000 loops, best of 11: 540 nsec per loop
$ pytime -s 'from random import randrange' 'randrange(15)'   # new versio
500000 loops, best of 11: 502 nsec per loop

It should become even faster when we get a zero-cost try.

It's odd that non-integers raise a ValueError. I think that should have been TypeError.

https://bugs.python.org/issue42222

@rhettinger rhettinger added the performance Performance or resource usage label Oct 31, 2020
serhiy-storchaka
serhiy-storchaka reviewed Oct 31, 2020
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Currently you are on the way to #19112 (which already has tests and documentation and cover corner cases).

@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 31, 2020
Merged
@github-actions
Copy link

github-actions bot commented Dec 16, 2020

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 16, 2020
@rhettinger rhettinger removed the stale Stale PR or inactive for long period of time. label Dec 24, 2020
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 24, 2020

What about #19112?

@rhettinger
Copy link
Contributor Author

rhettinger commented Dec 24, 2020

I merged in the relevant parts of #19112. The principal differences are:

  • The TypeError won't start being emitted until the after the deprecation period.
  • Improved wording for the doc entries.
  • Tests focus solely on the deprecations.
  • All existing behavior remains intact until after the deprecation.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 24, 2020

  • The TypeError won't start being emitted until the after the deprecation period.

As in #19112 initially. I changed ValueError to TypeError on your request.

  • Improved wording for the doc entries.

I would appreciate any suggestions to change wording.

  • Tests focus solely on the deprecations.

Yes, tests in #19112 cover more cases.

  • All existing behavior remains intact until after the deprecation.

As well as in #19112.

But this PR still has issues. It does not emit warning for randrange(10, 20, 1.0).

@rhettinger rhettinger merged commit a9621bb into python:master Dec 28, 2020
@bedevere-bot
Copy link

bedevere-bot commented Dec 28, 2020

@rhettinger: Please replace # with GH- in the commit message next time. Thanks!

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@rhettinger @adorilson
bpo-42222: Modernize integer test/conversion in randrange() (python#2…
12cdba8 
…3064)
@AlexWaygood AlexWaygood mentioned this pull request Feb 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@tim-one tim-one Awaiting requested review from tim-one

Assignees

No one assigned

Labels

performance Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rhettinger @serhiy-storchaka @bedevere-bot @the-knights-who-say-ni