From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <archer-return-1815-listarch-archer=sourceware.org@sourceware.org>
Received: (qmail 31099 invoked by alias); 17 Dec 2009 14:34:32 -0000
Mailing-List: contact archer-help@sourceware.org; run by ezmlm
Sender: <archer@sourceware.org>
Precedence: bulk
List-Post: <mailto:archer@sourceware.org>
List-Help: <mailto:archer-help@sourceware.org>
List-Subscribe: <mailto:archer-subscribe@sourceware.org>
List-Id: <archer.sourceware.org>
Received: (qmail 31088 invoked by uid 22791); 17 Dec 2009 14:34:31 -0000
X-SWARE-Spam-Status: No, hits=-2.0 required=5.0
	tests=BAYES_00,SARE_MSGID_LONG40,SPF_PASS
X-Spam-Check-By: sourceware.org
MIME-Version: 1.0
Reply-To: matt@mmmccormick.com
In-Reply-To: <4B29F2FF.8090408@redhat.com>
References: <f23d1cbc0912142151s57c2442fqb05cc4af729b4b71@mail.gmail.com>
	 <1260856483-24773-1-git-send-email-matt@mmmccormick.com>
	 <4B29F2FF.8090408@redhat.com>
Date: Thu, 17 Dec 2009 14:34:00 -0000
Message-ID: <f23d1cbc0912170634i7c8c9e5cy5f960c3312c58540@mail.gmail.com>
Subject: Re: [PATCH 1/2] [python] Add gdb.value_history_count()
From: "Matthew McCormick (thewtex)" <matt@mmmccormick.com>
To: Phil Muldoon <pmuldoon@redhat.com>
Cc: archer@sourceware.org
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
X-SW-Source: 2009-q4/txt/msg00117.txt.bz2

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. =A0This is not an
> official review though, Tom will have to look at it.
>
>
>> 2009-12-14 =A0Matt McCormick =A0<matt@mmmccormick.com>
>>
>> =A0 =A0 =A0 * gdb/python/py-value.c (gdbpy_value_history_count): Impleme=
nt
>> =A0 =A0 =A0 gdb.value_history_count()
>> =A0 =A0 =A0 * gdb/python/python-internal.h (thread_object): Python exten=
sion
>> =A0 =A0 =A0 definition.
>> =A0 =A0 =A0 * gdb/python/python.c (GdbMethods): Register in module metho=
ds.
>> =A0 =A0 =A0 * gdb/value.c (get_value_history_count): New Function.
>> =A0 =A0 =A0 * gdb/value.h (get_value_history_count): New Function.
>
>
> The paths here seem wrong. =A0For items in the python directory, the
> entries belong in the ChangeLog in the src/gdb directory. =A0It should
> read:
>
>
> 2009-12-14 =A0Matt McCormick =A0<matt@mmmccormick.com>
>
> =A0 =A0 =A0 =A0* python/py-value.c (gdbpy_value_history_count): Implement
> =A0 =A0 =A0 =A0gdb.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)
>> +{
>> + =A0return Py_BuildValue("i", get_value_history_count());
>> +}
>
>
> Similarly a space before '(' in a function. =A0So:
>
>
>> + =A0return 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.
>
>
>> =A0/* Accessor methods. =A0*/
>>
>> +int
>> +get_value_history_count()
>> +{
>> + =A0return 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. =A0Two 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. =A0This also helps catch regressions in the
> future. =A0This 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.