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

Metzger, Markus T
2017-09-21T16:53:02Z

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Thursday, September 21, 2017 3:47 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,

> > My reason is simply to not mix different styles.
> >
>
> Alright, that's certainly a valid reason.
>
> My view is that declarations in the middle of blocks are
> desirable, and enforcing top-of-block-only consistency too
> strongly prevents incremental modernization a bit, since it
> puts the bar higher (because either everything is converted,
> or else nothing is). So personally I'm fine with mixed style,
> and I try to push for declare-at-initialization when it
> makes sense.

I find top declarations more readable but I see the need for
C++ objects. And I value consistency. So let's go with mixed
style everywhere.

> Ack. To me, if we need a simpler rationale, it could go
> like this: middle-of-block declaration+initialization makes
> sense with non-trivial types. And if we accept middle-of-block
> declarations with non-trivial types, then there's no good
> rationale for not allowing them with scalar types too.

Agreed.

> > So please ignore my comments on declaration placement.
>
> On the contrary, it's not my intention to make your comment
> be ignored, rather that we all discuss this and end up on
> the same page. So thanks for the discussion, and really sorry
> if I sounded too nit picky.

Tom still needs to ignore my previous comment on declaration
placement;-) In this and in other patches of this series.

>
> -----
>
> BTW, looking at the get_context_size function in question, it
> seems like the 'number' variable is not used, meaning we could
> apply something like this:
>
> static int
> get_context_size (char **arg)
> {
> - char *pos;
> - int number;
> -
> - pos = skip_spaces (*arg);
> + char *pos = skip_spaces (*arg);
>
> if (!isdigit (*pos))
> error (_("Expected positive number, got: %s."), pos);
>
> return strtol (pos, arg, 10);
> }

Tom's patch changed the parameter to const char so we still need the
char *end detour. You're right, though, that NUMBER is not used and
that the declaration can be removed.

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