[patch 3/4] hw watchpoints made multi-inferior
Yao Qi
yao@codesourcery.com
Mon Dec 13 02:47:00 GMT 2010
On 12/11/2010 01:15 PM, Jan Kratochvil wrote:
>
>>> * linux-nat.c (linux_nat_iterate_watchpoint_lwps): Fix
>>> iterate_over_lwps FILTER.
>>
>> What is FILTER?
>
> `filter' is name of the first parameter of iterate_over_lwps.
>
>> Why capitalized?
>
> I follow:
>
> http://sourceware.org/ml/gdb-patches/2008-09/msg00374.html
> On Wed, 17 Sep 2008 20:03:37 +0200, Daniel Jacobowitz wrote:
> # You capitalize FOO when you mean "the value of a variable named foo", but the
> # name of the variable is still "foo".
>
> But I haven't found such rule in GNU Coding Standards (GCS) now. But some
> keywords are capitalized in GCS.
>
IMO, this rule is for comments of function header, and I don't see it in
ChangeLog entries before. Thanks for this explanation. It looks OK to me.
>>> + if (old_loc->pspace != current_program_space)
>>> + continue;
>>
>> code indent problem? Put extra three spaces in front of "continue".
>
> There is <tab> before `continue;'. The tab should be there for any 8 spaces
> according to GNU indent options present in GCS.
>
> It just gets displayed wrong in the patch file due to the first column from
> diff. I do not know an easy solution for it. Rather well formatted code
> using >=4 blocks indentation looks wrong to me (as it may not be using tabs).
>
OK, no problem.
>
>>> +static void
>>> +i386_inferior_data_cleanup (struct inferior *inf, void *arg)
>>> +{
>>> + struct i386_inferior_data *inf_data = arg;
>>> +
>>> + xfree (inf_data);
>>> +}
>>
>> Can't we 'xfree (arg);' directly?
>
> This was discussed here before at least in:
> http://sourceware.org/ml/gdb-patches/2010-05/msg00162.html
>
> The problem is primarily GDB still has not switched to C++ with templates
> where ARG could have the right type instead of `void *'.
>
> With the `void *' cast I find it both more readable to see what the parameter
> in fact is. And I also find it safer is someone needs to access some members
> of `struct i386_inferior_data' in the future. Having to re-cast `void *' to
> the right type during such code changes can be IMO more fragile than to just
> write it down once when one implements both the caller and the callback.
>
> And it is just the simpler stupid writing of code when one does not have to
> think about it much and it just works.
>
> Unless decided otherwise so far I consider this style is agreed upon for GDB.
Oh, yes. I didn't know it before. Thanks for your explanation.
>>> +if ![runto_main] {
>>
>> Please add `perror "Couldn't run ${testfile}"'
>>
>>> + return
>>> +}
>
> In any case runto_main returns 0 some kind of error message has been already
> printed. Or do I miss some case?
>
No. Let's keep it as it is now.
I have no other comments to your new patch.
--
Yao (é½å°§)
More information about the Gdb-patches
mailing list