Re: [patch] [python] find_line_pc_range

[Phil Muldoon <pmuldoon at redhat dot com>]

Matt Rice <ratmice@gmail.com> writes:

> Thanks, attached is an updated patch that also includes tests.

Thanks.

> +      ret_tuple = PyTuple_New (2);
> +      if (!ret_tuple)
> +        return Py_None;

Return NULL here.  This signifies an error, and that we have abandoned
the function.  When a Python function returns NULL, the exception is
eventually printed and cleared either by the callers, or by the GDB
Python top-level exception handling code.

> +      start = gdb_py_long_from_ulongest (start_pc);
> +      if (!start)
> +	goto fail_ret;
> +      if (PyTuple_SetItem (ret_tuple, 0, start) != 0)
> +	{
> +	  Py_XDECREF (start);
> +	  goto fail_ret;
> +	}

You do not need to decrement "start" here.  Even if the tuple SetItem
call failed, you have already passed the responsibility to the tuple,
and you no longer own it (stolen reference).  SetItem would decrement
this on failure.  So just goto to your local error block.

> +      end = gdb_py_long_from_ulongest (end_pc);
> +      if (!end)
> +	goto fail_ret;
> +      if (PyTuple_SetItem (ret_tuple, 1, end) != 0)
> +	{
> +	  Py_XDECREF (end);
> +	  goto fail_ret;
> +	}

Same as above.

> +      fail_ret:
> +        Py_XDECREF (ret_tuple);

Small nit, and this is generally ok, but you already know that if this
error block is called, ret_tuple will be instantiated. So in this case
the XDECREF is a tiny bit redundant.  You could just use DECREF here and
skip the NULL check that XDECREF does.  Like I said, a tiny nit, but I
just thought I would point it out.


> +	return Py_None;

If there is a Python exception that you cannot handle internally to your
function, you must always return NULL.  This informs callers, and
ultimately the exception managing code in GDB that some Python call
failed, to print the exception, and clear it.

> @@ -526,6 +573,8 @@ static PyMethodDef sal_object_methods[] = {
>    { "is_valid", salpy_is_valid, METH_NOARGS,
>      "is_valid () -> Boolean.\n\
>  Return true if this symbol table and line is valid, false if not." },
> +  { "find_line_pc_range", salpy_find_line_pc_range, METH_NOARGS,
> +    "Return a tuple of start and end ranges for the symbol table." },
>    {NULL}  /* Sentinel */
>  };

We try to put the return type in the help now as a formal statement.
Look at the two-line help example above: "is_valid()" for guidance.

Nearly there, thanks for the patch!

Cheers,

Phil