Re: [patch 3/4] hw watchpoints made multi-inferior

[Yao Qi <yao@codesourcery.com>]

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 (齐尧)