🌐 AI搜索 & 代理 主页
Skip to content

ipaddress: Use str.isascii() instead of frozenset. #5811

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
methane merged 4 commits into from
Feb 23, 2018
Merged

ipaddress: Use str.isascii() instead of frozenset. #5811

methane merged 4 commits into from
Feb 23, 2018

Conversation

@methane
Copy link
Member

@methane methane commented Feb 22, 2018

No description provided.

serhiy-storchaka
serhiy-storchaka reviewed Feb 22, 2018
View reviewed changes
Lib/ipaddress.py Outdated
raise ValueError("Only hex digits permitted in %r" % hextet_str)
# Reject non-ASCII characters, '+' and '-', since int() accepts them.
msg = "Only hex digits permitted in %r"
if not hextet_str.isascii() or not hextet_str.isalnum():
Copy link
Member

@serhiy-storchaka serhiy-storchaka Feb 22, 2018

Choose a reason for hiding this comment

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

'z' and '_' shouldn't be permitted.

Copy link
Member Author

@methane methane Feb 23, 2018

Choose a reason for hiding this comment

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

They are rejected by int()

Copy link
Member

@serhiy-storchaka serhiy-storchaka Feb 23, 2018

Choose a reason for hiding this comment

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

'0x12' is accepted by int().

Copy link
Member Author

@methane methane Feb 23, 2018

Choose a reason for hiding this comment

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

You're right. I'll revert it.

Lib/ipaddress.py Outdated
# int allows a leading +/- as well as surrounding whitespace,
# so we ensure that isn't the case
if not _BaseV4._DECIMAL_DIGITS.issuperset(prefixlen_str):
if not prefixlen_str.isascii() or not prefixlen_str.isdigit():
Copy link
Member

@serhiy-storchaka serhiy-storchaka Feb 22, 2018

Choose a reason for hiding this comment

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

Is _DECIMAL_DIGITS still used?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Feb 22, 2018

What is the benefit of rewriting the code?

@methane
Copy link
Member Author

methane commented Feb 23, 2018

What is the benefit of rewriting the code?

I found these code and I felt "Python should have str.isascii()". I proposed it and it's implemented.
That's why I create this pull request.

There are a little performance gain too.

./python -m perf timeit -s 'import ipaddress,importlib' -- 'importlib.reload(ipaddress)'
master:  Mean +- std dev: 1.95 ms +- 0.04 ms
isascii: Mean +- std dev: 1.88 ms +- 0.03 ms

./python -m perf timeit -s 'from ipaddress import ip_address' -- 'ip_address("10.1.2.3")'
master:  Mean +- std dev: 8.27 us +- 0.13 us
isascii: Mean +- std dev: 7.51 us +- 0.13 us

@methane
Copy link
Member Author

methane commented Feb 23, 2018

BTW, which is readable?

-        if not prefixlen_str.isascii() or not prefixlen_str.isdigit():
+        if not (prefixlen_str.isascii() and prefixlen_str.isdigit()):

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Feb 23, 2018

I would prefer the second form. Both are compiled to the same bytecode.

@methane methane merged commit 58a1096 into python:master Feb 23, 2018
@methane methane deleted the ipaddress-isascii branch February 23, 2018 11:02
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 approved these changes

Assignees

No one assigned

Labels

skip issue skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

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