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

Metzger, Markus T
2017-09-21T13:06:20Z

> -----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