classification
Title: Argument Clinic: add unsigned integers converters
Type: enhancement Stage: resolved
Components: Argument Clinic, Demos and Tools, Interpreter Core Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: georg.brandl, josh.r, larry, mark.dickinson, rmsr, serhiy.storchaka
Priority: low Keywords: patch

Created on 2014-01-14 17:31 by serhiy.storchaka, last changed 2018-07-26 11:35 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
clinic_unsigned_converters.patch serhiy.storchaka, 2014-01-14 17:31 review
clinic_unsigned_converters.patch serhiy.storchaka, 2014-01-15 18:59 review
Pull Requests
URL Status Linked Edit
PR 8434 merged serhiy.storchaka, 2018-07-24 07:37
Messages (15)
msg208102 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-14 17:31
Proposed patch adds support for non-bitwise unsigned integer arguments in Argument Clinic. I.e. now unsigned_int(bitwise=False) (or just unsigned_int) converts Python int in range from 0 to UINT_MAX to C unsigned int. Added support for unsigned_short, unsigned_int, unsigned_long, unsigned_PY_LONG_LONG, and size_t.

Also added global private functions _Py_UnsignedShort_Converter(), _Py_UnsignedInt_Converter(), _Py_UnsignedLong_Converter(), _Py_UnsignedLongLong_Converter(), and _Py_Size_t_Converter(), which are used by Argument Clinic and in C code still not converted to Argument Clinic.
msg208103 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-14 17:34
Very nice!  I had an idea to do this in the back of my head too.  But my plate is already full.

I haven't reviewed the patch yet--and I have a huge backlog of reviews already.  But I can give this higher priority if you need it for other patches.  When do you need this reviewed?
msg208107 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-14 18:29
Currently these converters are used only in zlib (unsigned int) and select (unsigned short). But perhaps during conversion to Argument Clinic we will discover that they are more appropriate than bitwise converters in other places. So there is no hurry.
msg208174 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-15 16:51
We can use this now.  So I bumped it to the top of my queue and reviewed it.  I had only minor feedback--thanks!
msg208180 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-15 18:59
Unfortunately I have discovered that there is significant difference between uint_converter in Modules/zlibmodule.c and proposed _PyLong_UnsignedInt_Converter. And there are tests in Lib/test/test_zlib.py which fail when _PyLong_UnsignedInt_Converter is used instead of uint_converter. uint_converter raises ValueError for negative integers, and _PyLong_UnsignedInt_Converter raises OverflowError.

Possible solutions:

1. Change tests. I don't like this, ValueError looks reasonable for these cases.

2. Continue to use uint_converter in Modules/zlibmodule.c. Then new converters become less useful.

3. Raise ValueError in new converters for negative integers. ValueError looks reasonable in many cases.

4. Raise ValueError in PyLong_AsUnsignedLong etc for negative integers.

Here is a patch which incorporates Larry's suggestions and implements option #3.
msg208181 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-15 19:00
Oh, and of course 5th option: do nothing.
msg208201 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2014-01-15 22:12
I think we cannot change PyLong_AsUnsignedLong() without a deprecation period,
if at all.  That leaves the option of changing the converters.

My preference is to raise either OverflowError or ValueError for *both*
out-of-range conditions.

People may me used to OverflowError by now -- the usage in PyLong_AsUnsignedLong()
dates back to very early revisions -- but there are equally good reasons to use
ValueError.
msg208229 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-16 02:36
I'm glad you caught that!  First things first: the converted code should behave identically to the existing code, including raising the same exceptions.


If you examine the exception hierarchy:
    http://docs.python.org/3.4/library/exceptions.html#exception-hierarchy

you'll see that "OverflowError" is a subclass of "ArithmeticError".  In other words, it represents when you perform an arithmetic operation that overflows the result type.  Using it to also represent "you specified a value that is out of range for this conversion" seems wrong.

So I like #3 as well.

Could _PyLong_UnsignedInt_Converter catch the OverflowError raised by PyLong_AsUnsignedLong and reraise it as ValueError?
msg208376 - (view) Author: Ryan Smith-Roberts (rmsr) * Date: 2014-01-18 00:28
socketmodule has three builtins which use PyLong_AsUnsignedLong on their arguments and would benefit from these converters, but only if they raise OverflowError. So I vote for #2.

I don't think it's unreasonable to continue to have locally-defined argument converters.
msg208730 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-22 01:40
Is this waiting on something?  I agree that we can't change the behavior
of existing functions.  But your new converters should raise ValueError.
msg208731 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-22 01:43
Also, you didn't remove the _ in front of "Converter" in the names, e.g "_PyLong_UnsignedShort_Converter" should be "_PyLong_UnsignedShortConverter".
msg208734 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-22 02:00
Actually, here's another data point to consider.  The underlying implementation of PyArg_Parse* is the function convertsimple() in Python/getargs.c.  For all the non-bitwise integer format units ('bhi'), if they overflow or underflow the native integer they raise OverflowError.
msg208778 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-01-22 11:04
I think that we do not yet have enough data. Too little cases are known for which unsigned integer converters are needed, and different cases require a different behavior. I prefer to defer until we convert the majority of the code. Then we will see what behavior is most prevalent. We even can change and unify behavior in 3.5.

There is no hurry. We can use in each module its own Converter (if needed). There should not be many such places.
msg209092 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-01-24 15:37
I can start citing data points if you like.

socket.if_indextoname just calls PyLong_AsUnsignedLong().  Not sure what that throws.
msg322411 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-26 10:22
New changeset 7cb7bcff20a386bba59cbc51e2419542de358bd2 by Serhiy Storchaka in branch 'master':
bpo-20260: Implement non-bitwise unsigned int converters for Argument Clinic. (GH-8434)
https://github.com/python/cpython/commit/7cb7bcff20a386bba59cbc51e2419542de358bd2
History
Date User Action Args
2018-07-26 11:35:25serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.8, - Python 3.4
2018-07-26 10:22:18serhiy.storchakasetmessages: + msg322411
2018-07-24 07:37:31serhiy.storchakasetpull_requests: + pull_request7959
2015-02-25 15:26:22serhiy.storchakasetcomponents: + Argument Clinic
2014-05-22 21:56:18josh.rsetnosy: + josh.r
2014-05-22 21:53:14skrahsetnosy: - skrah
2014-01-24 15:37:32larrysetmessages: + msg209092
2014-01-22 11:04:40serhiy.storchakasetpriority: normal -> low

messages: + msg208778
2014-01-22 02:00:08larrysetmessages: + msg208734
2014-01-22 01:43:29larrysetmessages: + msg208731
2014-01-22 01:40:20larrysetmessages: + msg208730
2014-01-18 00:28:44rmsrsetnosy: + rmsr
messages: + msg208376
2014-01-16 02:36:07larrysetmessages: + msg208229
2014-01-15 22:12:20skrahsetmessages: + msg208201
2014-01-15 19:00:43serhiy.storchakasetmessages: + msg208181
2014-01-15 18:59:40serhiy.storchakasetfiles: + clinic_unsigned_converters.patch
nosy: + mark.dickinson, skrah
messages: + msg208180

2014-01-15 16:51:27larrysetmessages: + msg208174
2014-01-14 18:29:33serhiy.storchakasetmessages: + msg208107
2014-01-14 17:34:46larrysetmessages: + msg208103
2014-01-14 17:31:58serhiy.storchakacreate