This is the mail archive of the archer@sourceware.org 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 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.
>

Thank you for the comments.  An updated patch will follow.

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

Thank you for the note.  I have looked at the GNU documentation on
ChangeLog's and some of your commits, and I will try to follow their
lead.  I am hacking together a git hook to prototype the commit
message correctly.  If anything else in the future is not as it should
be, let me know.

>
>> +/* Implementation of gdb.value_history_count. */
>
>
> For FSF C style, two spaces after the period, please.
>

ok.

>
>> +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 ());
>

ok.

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

Good point.  PyInt_AsLong is not appropriate since the argument is not
a PyObject *, but I changed it to

PyInt_FromLong ((long) get_value_history_count());

>
>> ?/* Accessor methods. ?*/
>>
>> +int
>> +get_value_history_count()
>> +{
>> + ?return value_history_count;
>> +}
>> +
>
>
> Space after '('.
>

ok.

>
>>
>> +/* 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. ;)
>

Fixed.

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

I coded a test case and submit that too.

Thanks,
Matt

>
> Cheers,
>
> Phil
>
>
>
>
>
>


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