[Patches] [ python-Patches-1436368 ] PEP 357 --- adding nb_index

Tue Mar 7 05:01:52 CET 2006

Patches item #1436368, was opened at 2006-02-21 20:35
Message generated for change (Comment added) made by teoliphant
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1436368&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: 5
Submitted By: Travis Oliphant (teoliphant)
Assigned to: Guido van Rossum (gvanrossum)
Summary: PEP 357 --- adding nb_index

Initial Comment:
See PEP 357 for details.

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

>Comment By: Travis Oliphant (teoliphant)
Date: 2006-03-06 21:01

Message:
Logged In: YES 
user_id=5296

I changed the wrap_index function so that in Pyhton the
__index__ method for long returns the object itself should
it be too large for a Py_ssizet.  I'm sorry I didn't
understand what you wanted before. 

* added Lib/test/test_index.py
* removed '\' used to split C-lines (where I could find them)
* put spaces around '==' sign
* check for an error at the start of _long_as_ssize_t so
that both _PyLong_AsSize_t() and long_index return an
error if one was pre-existing.  That way the errors are
carefully controlled in _long_assize_t and different things
are done (ignoring overflow or raising an error) depending
on the what calls _long_as_ssize_t.

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

Comment By: Guido van Rossum (gvanrossum)
Date: 2006-03-06 12:36

Message:
Logged In: YES 
user_id=6380

I'm sorry, but this still doesn't do what I think __index__
ought to do for a long: it ought to return self unchanged,
even if it's out of range for ssize_t.  IOW I want
(2**100).__index__() to return 2**100, not 2**31-1.  The
clipping should be done only on the conversion to ssize_t in
nb_index.

I reviewed the code that was new since my last patch.  Apart
from the above objection, it all looks good except:

- You seem to have lost the diffs for test_index.py; if you
do "svn add Lib/test/test_index.py" it will show up.

- Style nit: you don't have to use \ to split C lines
*unless* it's a multi-line CPP macro definition.

- Another style nit: try to put spaces around the ==
operator (and other comparisons).  i==-1 looks like
line-noise to me; i == -1 looks much better.

- _PyLong_AsSize_t() and long_index check for
PyErr_Occurred() after calling _long_as_ssize_t(); they
should really cal PyErr_Clear() before calling that, since
otherwise they can be fooled by pre-existing errors. 
(Clearing pre-existing errors is *also* bad, but they are
really a symptom of something bad already going on; the
general rule is to pre-clear errors *if* you're going to
check for PyErr_Occurred() instead of checking for error
return values (NULL or -1).

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

Comment By: Travis Oliphant (teoliphant)
Date: 2006-03-06 11:27

Message:
Logged In: YES 
user_id=5296

This patch does not contain the bug in mmap that the last
one did. All regression tests for Linux pass.

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

Comment By: Travis Oliphant (teoliphant)
Date: 2006-03-06 03:27

Message:
Logged In: YES 
user_id=5296

Here's a new patch that does not raise overflow error for
(sys.maxint+1).__index__

This patch also cleans up the apply_slice code so to exhibit
the same behavior because it uses the same code.  This has
one minor difference in that -PY_SSIZE_T_MAX-1 (isn't this
correct?) is used instead of -PY_SSIZE_T_MAX when clipping
negative numbers. 

Also, versionadded tags were created and applied, and the
arraymodule was updated to use __index__ for slicing.

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

Comment By: Travis Oliphant (teoliphant)
Date: 2006-03-06 03:27

Message:
Logged In: YES 
user_id=5296

Here's a new patch that does not raise overflow error for
(sys.maxint+1).__index__

This patch also cleans up the apply_slice code so to exhibit
the same behavior because it uses the same code.  This has
one minor difference in that -PY_SSIZE_T_MAX-1 (isn't this
correct?) is used instead of -PY_SSIZE_T_MAX when clipping
negative numbers. 

Also, versionadded tags were created and applied, and the
arraymodule was updated to use __index__ for slicing.

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

Comment By: Martin v. LÃ¶wis (loewis)
Date: 2006-03-05 18:50

Message:
Logged In: YES 
user_id=21627

In Objects/classobject.c, instance_index should be cast to
lenfunc, not inquiry. Otherwise, it looks fine.

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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-03-03 10:45

Message:
Logged In: YES 
user_id=33168

The doc needs \versionadded tags.  I can add those after
checkin.

Should sequences in Modules/ be upgraded too?  mmap, array,
various db modules, bufferobject, rangeobject.

Travis already mentioned the TP_FLAGS_HAVE_INDEX check in
ceval.c.

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

Comment By: Guido van Rossum (gvanrossum)
Date: 2006-03-03 10:45

Message:
Logged In: YES 
user_id=6380

Totally right.

I have one more concern:

>>> import sys
>>> (sys.maxint+1).__index__()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: long int too large to convert to int
>>> 

whiile I agree that the C API ought to silently truncate
values to fit within the boundaries established by ssize_t,
I think that when calling __index__() from Python, it should
be allowed to return an unbounded value.

Anyway, I haven't had time to whip up code to implement
that, but here's a new patch that adds the test to ISINDEX()
-- thanks for catching that!

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

Comment By: Travis Oliphant (teoliphant)
Date: 2006-03-03 10:08

Message:
Logged In: YES 
user_id=5296

I think there is still a need for a HASINDEX check in
the ISINDEX definition in ceval.c

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

Comment By: Guido van Rossum (gvanrossum)
Date: 2006-03-02 22:12

Message:
Logged In: YES 
user_id=6380

OK, Travis, here's a new patch.  It corrects the issues I
mentioned before, and does some whitespace cleanup (like
what Neal mentioned).  The biggest change is definitely the
check for Py_TPFLAGS_HAVE_INDEX whenever the nb_index slot
is referenced.

You don't have to do anything; but if you do have more
changes, please throw away your old changes, sync to the
head, and apply my patch, before doing any more work.

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

Comment By: Guido van Rossum (gvanrossum)
Date: 2006-03-02 21:32

Message:
Logged In: YES 
user_id=6380

Travis -- I'm working on a new patch. Please don't work on
one at the same time for the new few hours!  (Until midnight
PST.)

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

Comment By: Guido van Rossum (gvanrossum)
Date: 2006-03-02 18:16

Message:
Logged In: YES 
user_id=6380

I promise I'll review this further tomorrow!

Feedback so far:

- compilation error with svn HEAD on abstract.c:650

- all the uses of the nb_index slot must check for some
Py_TPFLAGS_... bit before using that slot (see object.h)

- the comment in ceval.c about the clipping of values <
-PY_SSIZE_T_MAX is incorrect; they are boosted to
-PY_SSIZE_T_MAX, not to zero

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

Comment By: Travis Oliphant (teoliphant)
Date: 2006-02-22 10:52

Message:
Logged In: YES 
user_id=5296

I followed Neal's comments and updated a new patch.  The new
patch contains the tests and the documentation updates as well. 

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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-02-22 01:03

Message:
Logged In: YES 
user_id=33168

I see one problem in Modules/operator.c.  This code:

+	if (i==-1 && PyErr_Occurred()) return NULL;
+	else return PyInt_FromLong((long)i);

Should be:

+	if (i == -1 && PyErr_Occurred())
+               return NULL;
+	return PyInt_FromSsizeT(i);

There was a discussion on python-checkins recently about
this.  The short answer is that if Py_ssize_t's are cast to
a long, that will lose precision on Win64 (size_ts are
64-bits, but longs are still 32).  Since i is Py_ssize_t,
the cast would lose the high order values (above 2G).

In Objects/stringobject.c:
-			i += PyString_GET_SIZE(self);
-		return string_item(self,i);
+			i += PyList_GET_SIZE(self);
+		return string_item(self, i);

I don't see how that (PyList) can be correct, self is a
string object.

I would change this code:
+	lenfunc func;
+	PyNumberMethods *nb;
+	if ((nb=item->ob_type->tp_as_number) &&	\
+	    (func=nb->nb_index)) {				
+		Py_ssize_t i = func(item);

to:
+	PyNumberMethods *nb = item->ob_type->tp_as_number;
+	if (nb != NULL && nb->nb_index != NULL) {		+		Py_ssize_t i
= nb->nb_index(item);

Let the compiler do the work.  We can optimize later if this
is a problem.  I think the code is clearer this way and is
more similar to the rest of the code.  I don't care as much
about the use of func here as the change to nb = ...  If you
want to keep the local variable, I would prefer a better
name like nb_index instead of func.

There's no need for a \ after &&.

Most of these comments apply to Objects/listobject.c,
Objects/tupleobject.c, Objects/unicodeobject.c, PyNumber_Index.

In Objects/classobject.c, you don't check that indexstr is
non-NULL after assignment.  In instance_index(), remove the
cast (int) of the return result outcome.

In slot_nb_index() remove . from end of exception string. 
Note: you don't need the Py_DECREF() and return -1 in the
last if as it would be the same if it fell through.  Use
whichever version you think is clearer.  Either way is fine
with me.

In Include/abstract.h, do all the comments still say a C
integer is returned even though a Py_ssize_t is returned? 
If not, can you update your comment too.

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

Comment By: Neal Norwitz (nnorwitz)
Date: 2006-02-22 00:33

Message:
Logged In: YES 
user_id=33168

Travis, I briefly looked over the patches and have a few
comments.  It's easier if there's one big patch that
contains all the code, tests, and doc.  I noticed a second
patch that contains the tests, but I didn't see any doc updates.

I noticed some formatting which is not consistent with the
style of the surrounding code (I think).  Stuff like
n=var->field (no spaces) and

 if (XXX)   dosomething();
 else return 5;

Most places in the python code check for == NULL rather than
 the implicit check for 0.  Most places in code don't do
assignment in if (cond), though this last one is more variable.

I'll try to look this over in more detail soon.  But it
probably won't be until PyCon.  If you're there, maybe we
can discuss in person.  Also, if you have further PEP
updates, feel free to send them to me and I can check them in.

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

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