Re: [PATCH 1/2] [python] Add gdb.value_history_count()

[Phil Muldoon <pmuldoon@redhat.com>]

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