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

["Matthew McCormick (thewtex)" <matt@mmmccormick.com>]

On Thu, Dec 17, 2009 at 2:59 AM, Phil Muldoon <pmuldoon@redhat.com> wrote:
> 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
>

Thank you very much for the comments.  I'll make the changes and
submit an update.