I didn't have the heart to keep pestering Moshe, so I delayed a bit on the latest round (and then left town for a few days). As a result, I didn't get a chance to comment on this last patch.

There is one problem, and some formatting nits...

On Mon, 28 Feb 2000, Guido van Rossum wrote:

...

• static int instance_contains(PyInstanceObject *inst, PyObject *member)
• {
• static PyObject *__contains__;
• PyObject *func, *arg, *res;
• int ret;
• if(__contains__ == NULL) {

"Standard Guido Formatting" requires a space between the "if" and the "(". if, for, while, etc are not functions... they are language constructs.

...

• 	if(PyObject_Cmp(obj, member, &cmp_res) == -1)
  

• 		ret = -1;
  

• 	if(cmp_res == 0) 
  

• 		ret = 1;
  

• 	Py_DECREF(obj);
  

• 	if(ret)
  

• 		return ret;
  

I had suggested to Moshe to follow the logic in PySequence_Contains(), but he wanted to use PyObject_Cmp() instead. No biggy, but the above code has a bug:

PyObject_Cmp() does *not* guarantee a value for cmp_res if it returns -1. Therefore, it is possible for cmp_res to be zero despite an error being returned from PyObject_Cmp. Thus, you get a false-positive hit.

IMO, this section of code should read:

cmp_res = PyObject_Compare(obj, member); Py_XDECREF(obj); if (cmp_res == 0) return 1; if (PyErr_Occurred()) return -1;

The "ret" variable becomes unused and can be deleted. Oh! Just noted that "ret" is declared twice; one hiding the declaration of the other. With the above change and deletion of the inner "ret", then the hiding declaration problem is also fixed.

Cheers, -g