RE: [PATCH v3 1/3] gdb: Make global feature array a per-remote target array

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Pedro,

Thank you for the review.
The points you mention make sense to me. I added some remarks further down below.
 
> These shouldn't actually be translated, as GDB only accepts these in English.
> Like, in
> 
>   (gdb) set foo off
> 
> "off" is always "off".

Ah, ok. Thanks!

> > +      default:
> > +	return _("internal-error");
> 
> I'd just remove this one.

But we need some sort of default case, right?
I'd replace it with an assert as it's done in other locations, 
e.g. sth. like  gdb_assert_not_reached ("invalid var_auto_boolean");

> So this structure is now a bit odd.  It holds both packet description stuff, like
> the name/title, which is shared between all the remote connections; and
> configuration stuff, which is per-connection.  I think we should split this in
> two structs / arrays, so we only have one copy of the description stuff.
>

That makes sense to me but will cause quite some changes of the current patch.
I will go ahead with two structs and two arrays then.

> In fact, aren't show_cmd/set_cmd above essentially unused after this patch?
> I can only see places writing to those fields, I see nowhere actually reading
> from those fields.  I think you can just drop them entirely.

I agree.


> Missing space before parens in those two set_context calls.

Yes.

> > +
> > +  if (packet_idx >= 0 && packet_idx  < PACKET_MAX)
> 
> Spurious double space in "packet_idx  <".

Yes, thank you.

I will post a v4 of this series soon.

Best Regards,

Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928