This is the mail archive of the mailing list for the Archer project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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  <>
> 	* 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

2009-12-14  Matt McCormick  <>
  	* python/py-value.c (gdbpy_value_history_count): Implement

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?



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]