This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Patch to fix __index__() clipping
Type: Stage:
Components: Interpreter Core Versions: Python 2.5
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: nnorwitz Nosy List: arigo, ncoghlan, nnorwitz, teoliphant
Priority: critical Keywords: patch

Created on 2006-08-11 10:51 by teoliphant, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
patch_against_51208.diff teoliphant, 2006-08-11 10:51 Another patch to fix __index__() clipping
patch_against_51214.diff teoliphant, 2006-08-11 19:13 A second version of the patch.
Messages (8)
msg50860 - (view) Author: Travis Oliphant (teoliphant) * (Python committer) Date: 2006-08-11 10:51
This is a patch that builds off of Nick Coghlan's work
to fix the __index__() clipping problem.  

It defines 3 New C-API calls (1 is a macro):

int PyIndex_Check(obj) -- does this object have nb_index

PyObject* PyNumber_Index(obj) -- return nb_index(obj)
if possible

Py_ssize_t PyNumber_AsSsize_t(obj, err) -- return obj
as Py_ssize_t by frist going through nb_index(obj)
which returns an integer or long integer.  If err is
NULL, then clip on Overflow, otherwise raise err on
Overflow encountered when trying to fit the result of
nb_index into a Py_ssize_t slot. 

With these three C-API calls, I was able to fix all the
problems that have been identified and simplify several
pieces of code. 



msg50861 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2006-08-11 11:47
Logged In: YES 
user_id=1038590

The PyNumber_Index docs should explicitly state that it
raises IndexError when overflow occurs.

It may also be worth resurrecting _PyLong_AsClippedSsize_t
in order to clean up the implementation of
PyNumber_AsSsize_t (detecting OverflowError then poking
around in the guts of a long object is a bit ugly).

Other than that, looks good to me (I like this API a lot
better than mine).
msg50862 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2006-08-11 17:15
Logged In: YES 
user_id=4771

I like this API too, and the patch looks good
apart from some more details:

A style note first: some lines are indented with 
spaces instead of tabs.  This causes some
changes on otherwise-unchanged lines, too.

if PyIndex_Check(key)   =>   if (PyIndex_Check(key))

typeobject.c: slot_nb_index() may not need to do 
the type-checking because it is done anyway in 
the caller, which can only be PyNumber_Index().

classobject.c: same remark about instance_index().

unicodeobject.c: kill macro HAS_INDEX
mmapmodule.c:    idem
arraymodule.c:   idem

should operator.index(o) return 
PyNumber_AsSsize_t(o) or just PyNumber_Index(o)? 
I can think of use cases for the latter, which
is somehow the most primitive of the two, so it
should IMHO be exposed via the operator module.

docs: "possitive" => "positive"
msg50863 - (view) Author: Travis Oliphant (teoliphant) * (Python committer) Date: 2006-08-11 19:16
Logged In: YES 
user_id=5296

I've made the changes Armin suggested.  I changed
operator.index to go back to its original behavior of
returning a.__index__()

I'm +0 on adding _PyLong_AsClippedSsize_t to clean-up the
check for a negative long integer. 
msg50864 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2006-08-12 10:40
Logged In: YES 
user_id=4771

The check for a negative long needs to be done
differently; this is really depending too much
on internal details.

Note also that the _long_as_ssize_t() function
was introduced to support both long_index() and
_PyLong_AsSsize_t().  Now that the former is
removed, the situation looks slightly strange,
because _long_as_ssize_t() is doing a bit too
much work for its one remaining caller.
That's why somehow reusing _long_as_ssize_t()
from abstract.c would look cleaner to me.
msg50865 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2006-08-12 14:09
Logged In: YES 
user_id=1038590

It's probably worth grabbing just the part of my patch that
added the _PyLong_AsClippedSsize_t internal interface to
longobject.h - this used an output variable to indicate
whether or not clipping occurred, making it easy for
abstract.c to decide how to proceed.

No doubt some modifications would be needed to fit in with
Travis's patch, though, and I won't have time to do that
this week.
msg50866 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-08-12 16:53
Logged In: YES 
user_id=33168

I've modified this patch (mostly for style) and will be
checking in today.  Armin, I changed the calc for the sign
bit to use _PyLong_Sign().  I don't think this patch is
perfect (I added some XXX comments), but it needs to get in
sooner rather than later.  Please try to review what I check in.
msg50867 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2006-08-12 17:04
Logged In: YES 
user_id=33168

Committed revision 51236.

Please make comments on the checkin.  It would probably be
best to keep the discussion on python-dev.  Thanks!
History
Date User Action Args
2022-04-11 14:56:19adminsetgithub: 43807
2006-08-11 10:51:34teoliphantcreate