This is the mail archive of the
archer@sourceware.org
mailing list for the Archer project.
Re: [PATCH 1/2] [python] Add gdb.value_history_count()
On 12/15/2009 05:54 AM, Matt McCormick (thewtex) wrote:
On the whole this looks ok: a few nits from me. This is not an
official review though, Tom will have to look at it.
> 2009-12-14 Matt McCormick <matt@mmmccormick.com>
>
> * gdb/python/py-value.c (gdbpy_value_history_count): Implement
> gdb.value_history_count()
> * gdb/python/python-internal.h (thread_object): Python extension
> definition.
> * gdb/python/python.c (GdbMethods): Register in module methods.
> * gdb/value.c (get_value_history_count): New Function.
> * gdb/value.h (get_value_history_count): New Function.
The paths here seem wrong. For items in the python directory, the
entries belong in the ChangeLog in the src/gdb directory. It should
read:
2009-12-14 Matt McCormick <matt@mmmccormick.com>
* python/py-value.c (gdbpy_value_history_count): Implement
gdb.value_history_count()
and so on for all ChangeLog entries.
> +/* Implementation of gdb.value_history_count. */
For FSF C style, two spaces after the period, please.
> +PyObject *
> +gdbpy_value_history_count (PyObject *self, PyObject *args)
> +{
> + return Py_BuildValue("i", get_value_history_count());
> +}
Similarly a space before '(' in a function. So:
> + return Py_BuildValue ("i", get_value_history_count ());
While there is nothing wrong with Py_BuildValue, if it is certain that
the value is a long or an int, why not use PyLong_AsLong or PyInt_AsLong?
I'm purely curious.
> /* Accessor methods. */
>
> +int
> +get_value_history_count()
> +{
> + return value_history_count;
> +}
> +
Space after '('.
>
> +/* Abs number of last entry stored */
> +
> +int get_value_history_count();
> +
Comments need to be full sentences, with a period. Two spaces at the
end please. ;)
Even though this is pretty straight-forward, when I add any
functionality accessible to the user via the API, I like to add a test
to demonstrate it works. This also helps catch regressions in the
future. This should be pretty simple to code up. What do you think?
Cheers,
Phil