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

  ChangeLog
2017-09-20  Tom Tromey  <tom@tromey.com>

	* btrace.c (get_uint, get_context_size, no_chunk)
	(maint_btrace_packet_history_cmd)
	(maint_btrace_clear_packet_history_cmd, maint_btrace_clear_cmd)
	(maint_info_btrace_cmd): Constify.
---
 gdb/ChangeLog |  7 +++++++
 gdb/btrace.c  | 24 ++++++++++++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)
  

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Tom Tromey
> Sent: Thursday, September 21, 2017 7:10 AM
> To: gdb-patches@sourceware.org
> Cc: Tom Tromey <tom@tromey.com>
> Subject: [RFA 49/67] Constify some commands in btrace.c

Hello Tom,

> ChangeLog
> 2017-09-20  Tom Tromey  <tom@tromey.com>
> 
> 	* btrace.c (get_uint, get_context_size, no_chunk)
> 	(maint_btrace_packet_history_cmd)
> 	(maint_btrace_clear_packet_history_cmd, maint_btrace_clear_cmd)
> 	(maint_info_btrace_cmd): Constify.
> ---
>  gdb/ChangeLog |  7 +++++++
>  gdb/btrace.c  | 24 ++++++++++++++----------
>  2 files changed, 21 insertions(+), 10 deletions(-)

Looks good to me.


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

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
  

On 09/21/2017 08:02 AM, Metzger, Markus T wrote:

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

It's really up to you the style to use for this code as
btrace maintainer, but I hope that at least this clarifies
things for those that end up being asked different
things from different people.  :-)

Thanks,
Pedro Alves
  

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

On 09/21/2017 02:06 PM, Metzger, Markus T wrote:
>> From: Pedro Alves [mailto:palves@redhat.com]
> 
> Hello Pedro,

Hi Markus!

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

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

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.

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

-----

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);
 }

Thanks,
Pedro Alves
  

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