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

SourceForge.net noreply at sourceforge.net
Fri Mar 3 06:12:03 CET 2006

Patches item #1436368, was opened at 2006-02-21 22:35
Message generated for change (Comment added) made by gvanrossum
You can respond by visiting: 

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: Guido van Rossum (gvanrossum)
Date: 2006-03-03 00:12

Logged In: YES 

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 23:32

Logged In: YES 

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


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

Logged In: YES 

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 12:52

Logged In: YES 

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 03:03

Logged In: YES 

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);

+	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 02:33

Logged In: YES 

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: 

More information about the Patches mailing list