RE: [RFA 49/67] Constify some commands in btrace.c

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Thursday, September 21, 2017 1:12 PM
> To: Metzger, Markus T <markus.t.metzger@intel.com>; Tom Tromey
> <tom@tromey.com>; gdb-patches@sourceware.org
> Subject: Re: [RFA 49/67] Constify some commands in btrace.c

Hello Pedro,

> >> @@ -3213,13 +3214,16 @@ get_context_size (char **arg)
> >>    if (!isdigit (*pos))
> >>      error (_("Expected positive number, got: %s."), pos);
> >>
> >> -  return strtol (pos, arg, 10);
> >> +  char *end;
> >> +  long result = strtol (pos, &end, 10);
> >> +  *arg = end;
> >> +  return result;
> >>  }
> >
> > The rest of the declarations are at the beginning.  I'd prefer to keep it that way.
> 
> Interesting.  Out of curiosity, is there a particular reason for
> that preference?  I ask because we haven't adjusted our guidelines
> in this area yet, but I've been asking people to move
> declarations nearer where they're initialized in patch reviews.
> My reasoning for that has been that IMHO, declarations nearer to
> where they're initialized help with a few things:
> 
>  - helps with reasoning about the code, since the type of
>    the variable has more changes of being visible.  The
>    top of the scope may be a good number of lines above.
> 
>  - helps prevent accidental used-uninitialized bugs.
> 
>  - avoid gratuitous pessimization when the variable is of
>    non-trivial type, by avoiding separate default
>    construction + assignment steps.  (though that's not the
>    case here, since in this case all variables are scalars).
> 
> The point that I feel particularly strongly about is the
> last one, about non-trivial types, though as said that
> doesn't apply in this case.
> 
> For the first two points, in C89, we'd some times open a
> extra scope, like in this case you could write:
> 
>     {
>       char *end;
>       long result = strtol (pos, &end, 10);
> 
>       *arg = end;
>       return result;
>     }
> 
> In C99/C++, we can write declarations in the middle of
> blocks without forcing the extra scope.

My reason is simply to not mix different styles.


> It's really up to you the style to use for this code as
> btrace maintainer

That shouldn't be the case.  If GDB is moving from a separate
declaration block to mixed-in declarations we should do it
everywhere.

So please ignore my comments on declaration placement.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928