Campbell Barton wrote:

Hrvoje Niksic wrote:

...

...

double PyNumber_AsDouble(PyObject * value) { if (PyFloat_Check(value) || PyInt_Check(value)) { return PyFloat_AsDouble(value); } else { PyObject *pyfloat; double d;

  pyfloat = PyNumber_Float(value);
  d = PyNumber_AsDouble(pyfloat);

I assume this should be PyFloat_AsDouble?

...

  Py_XDECREF(pyfloat);
  return d;

} }

What happens if PyNumber_Float raises an exception? As far as I can tell, PyNumber_AsDouble will result in a "bad argument" exception being raised and will return -1, losing track of the original exception in the process.

Come to think of it, error checking problems could very well be the reason why the API doesn't provide a PyNumber_AsDouble.

It is a problem but I dont think thats a good enough reason not to have some way to do this since its such a common operation.

PyFloat_AsDouble cant return an error value, so enforcing that the pyobject is checked with PyNumber_Check seems reasonable to me.

To follow up on this issue, Iv added the function below to a branch of the Blender/Py API so Numbers that aren't pythons Ints of Floats work.

double EXPP_PyNumber_AsDouble(PyObject * value) { if (PyFloat_Check(value) || PyInt_Check(value)) { return PyFloat_AS_DOUBLE(value); } else { PyObject *pyfloat; double d;

	pyfloat = PyNumber_Float(value);
	d = PyFloat_AsDouble(pyfloat);
	Py_XDECREF(pyfloat);
	return d; /* will be -1 and set an error if its PyNumber_Float failed */
}

}

from looking at pythons source there are 2 ways to make sure this function succeeds.

I was under the impression the only way to do error checking for this was to do....

float f; if (!PyFloat_Check(pyob)) { PyErr_SetString(PyExc_TypeError, "float argument required"); return -1; } f = PyFloat_AS_DOUBLE(pyob)

But another way to do it is.... (used in python 2.5.1's stringobject.c:4150)

float f = PyFloat_Check(pyob); if (f == -1.0 && PyErr_Occurred()) { PyErr_SetString(PyExc_TypeError, "float argument required"); return -1; }

Which would work with EXPP_PyNumber_AsDouble to do error checking as well. so I think error checking is ok with that function. (and would like to see PyNumber_AsDouble in the C/Python API hint, hint)

However Im wondering if this is advisable, with PyErr_Occurred() its possible that some other part of the API (even a totaly separate API) sets the error string but dosnt return an error value. - Any suggestions?

Hrvoje Niksic wrote:

Campbell Barton <cbarton@metavr.com> writes:

...

  pyfloat = PyNumber_Float(value);

It might be a good idea to return -1 immediately if this returns NULL. That way you prevent clobbering the real exception with an "invalid argument" exception returned by PyFloat_AsDouble. I think your right, whats there should work but it could return -1 early.

If PyNumber_Float succeeds, but pyfloat turns out not to be float, then the invalid argument exception is perfectly fine.

Is this possible? - if its a number type I would have thaught it must be able to be converted to a float.

...

However Im wondering if this is advisable, with PyErr_Occurred() its possible that some other part of the API (even a totaly separate API) sets the error string but dosnt return an error value. - Any suggestions?

Requiring the caller to check for PyErr_Occurred seems like bad style, but I'm not sure that it's an actual problem, as long as it's clearly documented.

Lots of Pythons internal code and modules do this. see hows PyFloat_AsDouble's used in cfield.c, _struct.c, datetimemodule.c, selectmodule.c, socketmodule.c..... and others but look in those to see that its used. It looked like bad coding style to me too.

As far as I know, no part of the API is allowed to set the error string and not return an error value because that breaks other parts of Python. Consider this function:

static PyObject * test() { PyErr_SetString(PyExc_TypeError, "test"); Py_INCREF(Py_None); return Py_None; }

Calling it subtly breaks Python:

...

...

...

import foo foo.test() import sys Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: test

The exception lingered until the first function that relies on PyErr_Occurred() to test for an exception, in this case PyImport_*.

your example will mess things up but the way the API works (it seems), is you have functions to work around this and return the correct error values with functions that interface python.

PyFloat_AsDouble - assumes the type is a PyFloat, if its not compatible it WILL set the error string and leaves the API to see if the error string is set and return an error value (-1 or NULL depending on the type of function).

Campbell Barton <cbarton@metavr.com> writes:

...

If PyNumber_Float succeeds, but pyfloat turns out not to be float, then the invalid argument exception is perfectly fine.

Is this possible? - if its a number type I would have thaught it must be able to be converted to a float.

You're right, PyNumber_Float type-checks the return value of tp_as_number->nb_float. I thought I had checked previously and had seen otherwise, but I must have confused it with something else.

Campbell Barton wrote:

Hrvoje Niksic wrote:

...

...

double PyNumber_AsDouble(PyObject * value) { if (PyFloat_Check(value) || PyInt_Check(value)) { return PyFloat_AsDouble(value); } else { PyObject *pyfloat; double d;

  pyfloat = PyNumber_Float(value);
  d = PyNumber_AsDouble(pyfloat);

I assume this should be PyFloat_AsDouble?

...

  Py_XDECREF(pyfloat);
  return d;

} }

What happens if PyNumber_Float raises an exception? As far as I can tell, PyNumber_AsDouble will result in a "bad argument" exception being raised and will return -1, losing track of the original exception in the process.

Come to think of it, error checking problems could very well be the reason why the API doesn't provide a PyNumber_AsDouble.

It is a problem but I dont think thats a good enough reason not to have some way to do this since its such a common operation.

PyFloat_AsDouble cant return an error value, so enforcing that the pyobject is checked with PyNumber_Check seems reasonable to me.

To follow up on this issue, Iv added the function below to a branch of the Blender/Py API so Numbers that aren't pythons Ints of Floats work.

double EXPP_PyNumber_AsDouble(PyObject * value) { if (PyFloat_Check(value) || PyInt_Check(value)) { return PyFloat_AS_DOUBLE(value); } else { PyObject *pyfloat; double d;

	pyfloat = PyNumber_Float(value);
	d = PyFloat_AsDouble(pyfloat);
	Py_XDECREF(pyfloat);
	return d; /* will be -1 and set an error if its PyNumber_Float failed */
}

}

from looking at pythons source there are 2 ways to make sure this function succeeds.

I was under the impression the only way to do error checking for this was to do....

float f; if (!PyFloat_Check(pyob)) { PyErr_SetString(PyExc_TypeError, "float argument required"); return -1; } f = PyFloat_AS_DOUBLE(pyob)

But another way to do it is.... (used in python 2.5.1's stringobject.c:4150)

float f = PyFloat_Check(pyob); if (f == -1.0 && PyErr_Occurred()) { PyErr_SetString(PyExc_TypeError, "float argument required"); return -1; }

Which would work with EXPP_PyNumber_AsDouble to do error checking as well. so I think error checking is ok with that function. (and would like to see PyNumber_AsDouble in the C/Python API hint, hint)

However Im wondering if this is advisable, with PyErr_Occurred() its possible that some other part of the API (even a totaly separate API) sets the error string but dosnt return an error value. - Any suggestions?

Hrvoje Niksic wrote:

Campbell Barton <cbarton@metavr.com> writes:

...

  pyfloat = PyNumber_Float(value);

It might be a good idea to return -1 immediately if this returns NULL. That way you prevent clobbering the real exception with an "invalid argument" exception returned by PyFloat_AsDouble. I think your right, whats there should work but it could return -1 early.

If PyNumber_Float succeeds, but pyfloat turns out not to be float, then the invalid argument exception is perfectly fine.

Is this possible? - if its a number type I would have thaught it must be able to be converted to a float.

...

However Im wondering if this is advisable, with PyErr_Occurred() its possible that some other part of the API (even a totaly separate API) sets the error string but dosnt return an error value. - Any suggestions?

Requiring the caller to check for PyErr_Occurred seems like bad style, but I'm not sure that it's an actual problem, as long as it's clearly documented.

Lots of Pythons internal code and modules do this. see hows PyFloat_AsDouble's used in cfield.c, _struct.c, datetimemodule.c, selectmodule.c, socketmodule.c..... and others but look in those to see that its used. It looked like bad coding style to me too.

As far as I know, no part of the API is allowed to set the error string and not return an error value because that breaks other parts of Python. Consider this function:

static PyObject * test() { PyErr_SetString(PyExc_TypeError, "test"); Py_INCREF(Py_None); return Py_None; }

Calling it subtly breaks Python:

...

...

...

import foo foo.test() import sys Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: test

The exception lingered until the first function that relies on PyErr_Occurred() to test for an exception, in this case PyImport_*.

your example will mess things up but the way the API works (it seems), is you have functions to work around this and return the correct error values with functions that interface python.

PyFloat_AsDouble - assumes the type is a PyFloat, if its not compatible it WILL set the error string and leaves the API to see if the error string is set and return an error value (-1 or NULL depending on the type of function).

Campbell Barton <cbarton@metavr.com> writes:

...

If PyNumber_Float succeeds, but pyfloat turns out not to be float, then the invalid argument exception is perfectly fine.

Is this possible? - if its a number type I would have thaught it must be able to be converted to a float.

You're right, PyNumber_Float type-checks the return value of tp_as_number->nb_float. I thought I had checked previously and had seen otherwise, but I must have confused it with something else.