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

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

Hi,

Thank you for the review.

> On 2022-12-21 1:39 p.m., Christina Schimpe wrote:
> 
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -3,6 +3,20 @@
> >
> >  *** Changes since GDB 13
> >
> > +* Multi-target feature configuration
> > +
> > +  GDB now supports the individual configuration of remote target's
> > + feature
> 
> I think you meant for "remote target" to be plural, so it should be written with
> the approstrophe after the plural "s", like "remote targets'".  Otherwise, if you
> really meant singular, then an article seems missing after "of", as in
> 
>   "configuration of a/the remote target's"

Yes, I meant plural and fixed it with  "remote targets'". 

> > +  sets.  Based on the current selection of a target, the commands
> > + 'set remote  <name>-packet (on|off|auto)' and 'show remote
> > + <name>-packet' can be used to  configure a target's feature packet
> > + and to display its configuration,  respectively.
> > +
> > +  The configuration of the packet itself applies to the currently
> > + selected  target (if available).  If no target is selected, it
> > + applies to future remote  connections.  Similarly, the show commands
> > + print the configuration of the  currently selected target.  If no
> > + remote target is selected, the default  configuration for future connections is
> shown.
> > +
> >  * MI version 1 has been removed.
> 
> ...
> 
> >
> > +/* Description of a remote packet.  */
> > +
> > +struct packet_description
> > +  {
> 
> Please fix indentation of {, and then the struct's fields accordingly (and possibly
> reflow comments).  The "{" should be at column zero.  No need to repost the
> patch for this.

Fixed.

> > +    /* Name of the packet used for gdb output.  */
> > +    const char *name;
> > +
> > +    /* Title of the packet, used by the set/show remote name-packet
> > +       commands to identify the individual packages and gdb output.  */
> > +    const char *title;
> > +
> 
> Remove spurious empty line after last field.

Fixed.

> > +  };
> > +
> > +/* Configuration of a remote packet.  */
> > +
> > +struct packet_config
> > +  {
> 
> Ditto.

Fixed.

> > +    /* If auto, GDB auto-detects support for this packet or feature,
> > +       either through qSupported, or by trying the packet and looking
> > +       at the response.  If true, GDB assumes the target supports this
> > +       packet.  If false, the packet is disabled.  Configs that don't
> > +       have an associated command always have this set to auto.  */
> > +    enum auto_boolean detect;
> > +
> > +    /* Does the target support this packet?  */
> > +    enum packet_support support;
> > +  };
> > +
> This is OK with the nits above fixed.  Thank you!


I have pushed this change with the nits addressed.

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