Re: [PATCH v4 02/10] Make gdbserver reg_defs a vector of objects

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-03-23 10:54, Alan Hayward wrote:
>> Please keep the curly braces for the for, as shown here:
>> 
>> https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#index-multiple-variables-in-a-line
>> 
>> Can you put the moving of this function/making it static/removing the 
>> declaration
>> in its own patch?  It's pre-approved, with that fixed.
>> 
> 
> Ok, I’ve pushed this bit as requested:

Thanks!

> New version updated with all of the above.
> Checked this on X86 with make check on target board gdbserver.
> (Patch 3/10, and possibly others, will need updating too, but should
> be obvious).
> 
> Thanks for the review.
> 
> Alan.

I had some problems applying your patch, the tabs were replaced with 
spaces.  You can either fix the settings of your email client, or you 
can also use git-send-email when sending individual patch updates, like 
this:

   git send-email HEAD^ --subject-prefix="PATCH v4.1 02/10" --to <to> 
--in-reply-to <message-id>

where message-id can be found in the headers of the message you want to 
reply to (header Message-ID).  This allows the message to be correctly 
threaded.  It means the patch will be in a separate email than your 
response to the reviewer's comments, but I think that's fine.

Anyway, this particular one wasn't too difficult to fix up by hand.  It 
LGTM with one nit fixed:

> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
> index
> 262d03c0785f48e83b784b6177c52e2d253a6067..1f7861fd98dd9085c3fe9d7ee4b8396999060265
> 100644
> --- a/gdb/regformats/regdef.h
> +++ b/gdb/regformats/regdef.h
> @@ -21,6 +21,18 @@
> 
>  struct reg
>  {
> +  reg ()
> +    : name (""),
> +      offset (0),
> +      size (0)
> +  {}
> +
> +  reg (const char *_name, int _offset, int _size)

The offset value is always 0 initially, so you can remove it and 
initialize it to 0.

I think that this patch can also be pushed on its own, it's a good 
improvement regardless of the rest of the series.

Thanks,

Simon