Re: [PATCH] Python 3 support, part 1 (non-testsuite part)

[Tom Tromey <tromey@redhat.com>]

>>>>> "Paul" ==   <Paul_Koning@Dell.com> writes:

Paul> Here is an updated patch which I believe addresses all the comments.

Thanks.

Paul> Should I add a NEWS entry for the Python 3 support?

Yes, that would be good.

Paul>  PyTypeObject breakpoint_object_type =
Paul>  {
Paul> -  PyObject_HEAD_INIT (NULL)
Paul> -  0,				  /*ob_size*/
Paul> +    PyVarObject_HEAD_INIT (NULL, 0)

The new line has the wrong indentation.

Paul>  static PyTypeObject cmdpy_object_type =
Paul>  {
Paul> -  PyObject_HEAD_INIT (NULL)
Paul> -  0,				  /*ob_size*/
Paul> +    PyVarObject_HEAD_INIT (NULL, 0)

Ditto.

Paul>  PyTypeObject event_object_type =
Paul>  {
Paul> -  PyObject_HEAD_INIT (NULL)
Paul> -  0,                                          /* ob_size */
Paul> +    PyVarObject_HEAD_INIT (NULL, 0)

Ditto.

Paul>  static PyTypeObject eventregistry_object_type =
Paul>  {
Paul> -  PyObject_HEAD_INIT (NULL)
Paul> -  0,                                          /* ob_size */
Paul> +    PyVarObject_HEAD_INIT (NULL, 0)

Ditto.

Paul> +  if (except.reason < 0)
Paul> +    PyBuffer_Release (&pybuf);
Paul>    GDB_PY_HANDLE_EXCEPTION (except);
 
Paul> +#ifdef IS_PY3K
Paul> +  PyBuffer_Release (&pybuf);
Paul> +#endif

I think the first PyBuffer_Release also has to be conditional on IS_PY3K
here.

It seems like you could just always call it after the try-catch rather
than checking except.reason.

Paul> +#ifdef IS_PY3K
Paul> +static int

Blank line between these two.

Paul> @@ -638,6 +698,10 @@
Paul>      }
Paul>    GDB_PY_HANDLE_EXCEPTION (except);
 
Paul> +#ifdef IS_PY3K
Paul> +  PyBuffer_Release (&pybuf);
Paul> +#endif

It seems like PyBuffer_Release must be called before
GDB_PY_HANDLE_EXCEPTION here.

Paul> +#ifdef IS_PY3K
Paul> +static PyBufferProcs buffer_procs = {

Blank line between these two, and I think "{" on its own line is
generally preferred.

Paul> +++ gdb/python/py-value.c	14 Nov 2012 15:57:17 -0000
Paul> @@ -106,7 +106,7 @@
 
Paul>    Py_XDECREF (self->dynamic_type);
 
Paul> -  self->ob_type->tp_free (self);
Paul> +  obj->ob_type->tp_free (self);
Paul>  }
 
Doesn't this need the new Py_TYPE macro?

Paul> +/* FIXME: there are a lot of calls below that do not check the return
Paul> +   value for errors.  */

Don't bother with the new FIXME.

Paul> +  int python_init_ok = 0;

This seems to be unused.

Paul> +  progname_copy = PyMem_Malloc ((progsize + 1) * sizeof (wchar_t));
[...]
Paul> +  Py_SetProgramName (progname_copy);

I think a comment here explaining why we never free progname_copy would
be nice to have.

thanks,
Tom