Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-32582: chr() doesn't raise OverflowError anymore#5218

Closed
vstinner wants to merge 3 commits into
python:masterpython/cpython:masterfrom
vstinner:chr_overflowCopy head branch name to clipboard
Closed

bpo-32582: chr() doesn't raise OverflowError anymore#5218
vstinner wants to merge 3 commits into
python:masterpython/cpython:masterfrom
vstinner:chr_overflowCopy head branch name to clipboard

Conversation

@vstinner

@vstinner vstinner commented Jan 17, 2018

Copy link
Copy Markdown
Member

chr() now catchs OverflowError and raises a ValueError exception
instead.

https://bugs.python.org/issue32582

chr() now catchs OverflowError and raises a ValueError exception
instead.
Comment thread Python/bltinmodule.c Outdated
return NULL;
}
PyErr_Clear();
ch = -1;

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.

We don't need this line, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, right. I removed this useless assignment.

My intent was to get a different error message for underflow (< 0) or overflow (> 0). But in fact, the same error message for all cases is just fine.

@vstinner

Copy link
Copy Markdown
Member Author

Hum, this PR changes the exception type for large float.

Patched:

>>> chr(2.0**32)
ValueError: chr() arg not in range(0x110000)

Python 3.6:

>>> chr(2.0**32)
TypeError: integer argument expected, got float

@zhangyangyu

Copy link
Copy Markdown
Member

Ahh, yes. And now it could accept small float while not the case before. So still need to call "PyArg_Parse" and catch OverflowError then.

@serhiy-storchaka serhiy-storchaka 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.

Needed updating the chr() documentation an an entry in the What's New document, in the section "Porting to 3.7".

Comment thread Lib/test/test_builtin.py
self.assertRaises(ValueError, chr, -1)
self.assertRaises(ValueError, chr, 0x00110000)
self.assertRaises((OverflowError, ValueError), chr, 2**32)
self.assertRaises(ValueError, chr, -2**100)

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.

Use 2**1000. It is possible that once Python will run platform with 128-bit C long.

Comment thread Python/bltinmodule.c
/*[clinic end generated code: output=d34f25b8035a9b10 input=f919867f0ba2f496]*/
{
return PyUnicode_FromOrdinal(i);
int ch = _PyLong_AsInt(i);

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 is better to use PyLongAsLongAndOverflow().

@@ -0,0 +1,2 @@
The :func:`chr` builtin function now catchs :exc:`OverflowError` and raises

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.

"catches" -- this is an implementation detail. And I think it is better to not raise and catch it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.