[Patches] [ python-Patches-1538606 ] Patch to fix __index__() clipping

[SourceForge.net]

Patches item #1538606, was opened at 2006-08-11 03:51
Message generated for change (Comment added) made by nnorwitz
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1538606&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Core (C code)
Group: Python 2.5
Status: Open
Resolution: None
Priority: 9
Submitted By: Travis Oliphant (teoliphant)
Assigned to: Neal Norwitz (nnorwitz)
Summary: Patch to fix __index__() clipping

Initial Comment:
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. 





----------------------------------------------------------------------

>Comment By: Neal Norwitz (nnorwitz)
Date: 2006-08-12 09:53

Message:
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.

----------------------------------------------------------------------

Comment By: Nick Coghlan (ncoghlan)
Date: 2006-08-12 07:09

Message:
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.

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2006-08-12 03:40

Message:
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.

----------------------------------------------------------------------

Comment By: Travis Oliphant (teoliphant)
Date: 2006-08-11 12:16

Message:
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. 

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2006-08-11 10:15

Message:
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"


----------------------------------------------------------------------

Comment By: Nick Coghlan (ncoghlan)
Date: 2006-08-11 04:47

Message:
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).

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1538606&group_id=5470