[00/16] Add styling to the gdb CLI and TUI

  > From: Tom Tromey <tom@tromey.com>
> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org
> Date: Mon, 04 Mar 2019 10:40:46 -0700
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> Eli> What do you think about the idea to add a convenience variable that
> Eli> would provide the GDB version?
> 
> Seems reasonable to me.

How about the patch below?  Is it okay to go in?  (Note that I took
this opportunity to clean up whitespace in top.c, I hope it's OK to do
that as part of unrelated code changes.)
  

Ping!

> Date: Wed, 06 Mar 2019 18:02:00 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: gdb-patches@sourceware.org
> 
> > From: Tom Tromey <tom@tromey.com>
> > Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org
> > Date: Mon, 04 Mar 2019 10:40:46 -0700
> > 
> > >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > Eli> What do you think about the idea to add a convenience variable that
> > Eli> would provide the GDB version?
> > 
> > Seems reasonable to me.
> 
> How about the patch below?  Is it okay to go in?  (Note that I took
> this opportunity to clean up whitespace in top.c, I hope it's OK to do
> that as part of unrelated code changes.)
> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ac61e65..f2915d0 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>
> +
> +	* NEWS: Announce $_gdb_version.
> +
> +	* top.c (init_gdb_version_var): New function.
> +	(gdb_init): Call init_gdb_version_var.
> +
>  2019-03-06  Tom Tromey  <tromey@adacore.com>
>  
>  	* remote-sim.c (gdbsim_target_open): Use result of
> diff --git a/gdb/NEWS b/gdb/NEWS
> index cc7c35c..260e6cc 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,12 @@
>  
>  *** Changes since GDB 8.3
>  
> +* New built-in convenience variable $_gdb_version provides the GDB
> +  version.  It is handy for conditionally using features available
> +  only in or since specific GDB versions, in scripts that should work
> +  error-free with many different versions, such as in system-wide init
> +  files.
> +
>  *** Changes in GDB 8.3
>  
>  * GDB and GDBserver now support access to additional registers on
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index 0380322..313a061 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>
> +
> +	* gdb.texinfo (Convenience Vars): Document $_gdb_version.
> +
>  2019-03-05  Simon Marchi  <simon.marchi@efficios.com>
>  
>  	* python.texi (Values From Inferior): Change synopsys of the
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f2028f8..9d15337 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -11197,7 +11197,7 @@
>  @vindex $_tlb@r{, convenience variable}
>  The variable @code{$_tlb} is automatically set when debugging
>  applications running on MS-Windows in native mode or connected to
> -gdbserver that supports the @code{qGetTIBAddr} request. 
> +gdbserver that supports the @code{qGetTIBAddr} request.
>  @xref{General Query Packets}.
>  This variable contains the address of the thread information block.
>  
> @@ -11211,6 +11211,17 @@
>  @item $_gthread
>  The global number of the current thread.  @xref{global thread numbers}.
>  
> +@item $_gdb_version
> +@vindex $_gdb_version@r{, convenience variable}
> +The version of the running @value{GDBN}.  The value is an integer
> +number that encodes the major and minor @value{GDBN} versions as
> +@w{@code{@var{major}*100 + @var{minor}}}, so, e.g., @value{GDBN}
> +version 9.10 will produce the value @code{910}.  Development snapshots
> +and pretest versions have their minor version incremented by one;
> +thus, @value{GDBN} pretest 9.11.90 will produce the value 912.  This
> +variable allows you to write scripts that work with different versions
> +of @value{GDBN} without errors caused by features unavailable in some
> +of those versions.
>  @end table
>  
>  @node Convenience Funs
> diff --git a/gdb/top.c b/gdb/top.c
> index 22e6f7e..97b349a 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -147,22 +147,22 @@ int server_command;
>  
>  /* Timeout limit for response from target.  */
>  
> -/* The default value has been changed many times over the years.  It 
> -   was originally 5 seconds.  But that was thought to be a long time 
> +/* The default value has been changed many times over the years.  It
> +   was originally 5 seconds.  But that was thought to be a long time
>     to sit and wait, so it was changed to 2 seconds.  That was thought
> -   to be plenty unless the connection was going through some terminal 
> +   to be plenty unless the connection was going through some terminal
>     server or multiplexer or other form of hairy serial connection.
>  
> -   In mid-1996, remote_timeout was moved from remote.c to top.c and 
> +   In mid-1996, remote_timeout was moved from remote.c to top.c and
>     it began being used in other remote-* targets.  It appears that the
>     default was changed to 20 seconds at that time, perhaps because the
>     Renesas E7000 ICE didn't always respond in a timely manner.
>  
>     But if 5 seconds is a long time to sit and wait for retransmissions,
> -   20 seconds is far worse.  This demonstrates the difficulty of using 
> +   20 seconds is far worse.  This demonstrates the difficulty of using
>     a single variable for all protocol timeouts.
>  
> -   As remote.c is used much more than remote-e7000.c, it was changed 
> +   As remote.c is used much more than remote-e7000.c, it was changed
>     back to 2 seconds in 1999.  */
>  
>  int remote_timeout = 2;
> @@ -188,9 +188,9 @@ int (*deprecated_ui_loop_hook) (int);
>  
>  /* Called from print_frame_info to list the line we stopped in.  */
>  
> -void (*deprecated_print_frame_info_listing_hook) (struct symtab * s, 
> +void (*deprecated_print_frame_info_listing_hook) (struct symtab * s,
>  						  int line,
> -						  int stopline, 
> +						  int stopline,
>  						  int noerror);
>  /* Replaces most of query.  */
>  
> @@ -237,7 +237,7 @@ ptid_t (*deprecated_target_wait_hook) (ptid_t ptid,
>  /* Used by UI as a wrapper around command execution.  May do various
>     things like enabling/disabling buttons, etc...  */
>  
> -void (*deprecated_call_command_hook) (struct cmd_list_element * c, 
> +void (*deprecated_call_command_hook) (struct cmd_list_element * c,
>  				      const char *cmd, int from_tty);
>  
>  /* Called when the current thread changes.  Argument is thread id.  */
> @@ -1339,8 +1339,9 @@ There is NO WARRANTY, to the extent permitted by law.");
>  resources online at:\n    <http://www.gnu.org/software/gdb/documentation/>."));
>    fprintf_filtered (stream, "\n\n");
>    fprintf_filtered (stream, _("For help, type \"help\".\n"));
> -  fprintf_filtered (stream, _("Type \"apropos word\" to search for \
> -commands related to \"word\"."));
> +  fprintf_filtered (stream,
> +		    _("Type \"apropos word\" to search for commands \
> +related to \"word\"."));
>  }
>  
>  /* Print the details of GDB build-time configuration.  */
> @@ -1608,7 +1609,7 @@ quit_force (int *exit_arg, int from_tty)
>  
>    undo_terminal_modifications_before_exit ();
>  
> -  /* An optional expression may be used to cause gdb to terminate with the 
> +  /* An optional expression may be used to cause gdb to terminate with the
>       value of that expression.  */
>    if (exit_arg)
>      exit_code = *exit_arg;
> @@ -1998,11 +1999,21 @@ set_history_filename (const char *args,
>       directories the file written will be the same as the one
>       that was read.  */
>    if (!IS_ABSOLUTE_PATH (history_filename))
> -    history_filename = reconcat (history_filename, current_directory, "/", 
> +    history_filename = reconcat (history_filename, current_directory, "/",
>  				 history_filename, (char *) NULL);
>  }
>  
>  static void
> +init_gdb_version_var (void)
> +{
> +  struct internalvar *version_var = create_internalvar ("_gdb_version");
> +  int vmajor = 0, vminor = 0, vrevision = 0;
> +  sscanf (version, "%d.%d.%d", &vmajor, &vminor, &vrevision);
> +  set_internalvar_integer (version_var,
> +			   vmajor * 100 + vminor + (vrevision > 0));
> +}
> +
> +static void
>  init_main (void)
>  {
>    struct cmd_list_element *c;
> @@ -2206,4 +2217,7 @@ gdb_init (char *argv0)
>       prefix to be installed.  Keep things simple and just do final
>       script initialization here.  */
>    finish_ext_lang_initialization ();
> +
> +  /* Create $_gdb_version convenience variable.  */
> +  init_gdb_version_var ();
>  }
>
  

On 2019-03-06 11:02 a.m., Eli Zaretskii wrote:
>> From: Tom Tromey <tom@tromey.com>
>> Cc: Tom Tromey <tom@tromey.com>,  gdb-patches@sourceware.org
>> Date: Mon, 04 Mar 2019 10:40:46 -0700
>>
>>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
>>
>> Eli> What do you think about the idea to add a convenience variable that
>> Eli> would provide the GDB version?
>>
>> Seems reasonable to me.
> 
> How about the patch below?  Is it okay to go in?  (Note that I took
> this opportunity to clean up whitespace in top.c, I hope it's OK to do
> that as part of unrelated code changes.)

Ah, I noticed this because you changed the subject in your ping :).  This would
be very useful, especially that we can quite easily query this from MI as well
as Python.

> 
> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ac61e65..f2915d0 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,10 @@
> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>
> +
> +	* NEWS: Announce $_gdb_version.
> +
> +	* top.c (init_gdb_version_var): New function.
> +	(gdb_init): Call init_gdb_version_var.
> +
>  2019-03-06  Tom Tromey  <tromey@adacore.com>
>  
>  	* remote-sim.c (gdbsim_target_open): Use result of
> diff --git a/gdb/NEWS b/gdb/NEWS
> index cc7c35c..260e6cc 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,12 @@
>  
>  *** Changes since GDB 8.3
>  
> +* New built-in convenience variable $_gdb_version provides the GDB
> +  version.  It is handy for conditionally using features available
> +  only in or since specific GDB versions, in scripts that should work
> +  error-free with many different versions, such as in system-wide init
> +  files.
> +
>  *** Changes in GDB 8.3
>  
>  * GDB and GDBserver now support access to additional registers on
> diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
> index 0380322..313a061 100644
> --- a/gdb/doc/ChangeLog
> +++ b/gdb/doc/ChangeLog
> @@ -1,3 +1,7 @@
> +2019-03-06  Eli Zaretskii  <eliz@gnu.org>
> +
> +	* gdb.texinfo (Convenience Vars): Document $_gdb_version.
> +
>  2019-03-05  Simon Marchi  <simon.marchi@efficios.com>
>  
>  	* python.texi (Values From Inferior): Change synopsys of the
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index f2028f8..9d15337 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -11197,7 +11197,7 @@
>  @vindex $_tlb@r{, convenience variable}
>  The variable @code{$_tlb} is automatically set when debugging
>  applications running on MS-Windows in native mode or connected to
> -gdbserver that supports the @code{qGetTIBAddr} request. 
> +gdbserver that supports the @code{qGetTIBAddr} request.
>  @xref{General Query Packets}.
>  This variable contains the address of the thread information block.

This looks like an unintended change.

> @@ -11211,6 +11211,17 @@
>  @item $_gthread
>  The global number of the current thread.  @xref{global thread numbers}.
>  
> +@item $_gdb_version
> +@vindex $_gdb_version@r{, convenience variable}
> +The version of the running @value{GDBN}.  The value is an integer
> +number that encodes the major and minor @value{GDBN} versions as
> +@w{@code{@var{major}*100 + @var{minor}}}, so, e.g., @value{GDBN}
> +version 9.10 will produce the value @code{910}.  Development snapshots
> +and pretest versions have their minor version incremented by one;
> +thus, @value{GDBN} pretest 9.11.90 will produce the value 912.  This
> +variable allows you to write scripts that work with different versions
> +of @value{GDBN} without errors caused by features unavailable in some
> +of those versions.
>  @end table

I know we plan to move to a version scheme where we don't have a "patch"
number (a third number), but just in case, maybe we could plan for it anyway
just in case it ever changes again in the future (I don't expect it will,
but we never know.

So something like MAJOR * 10000 + MINOR * 100 + PATCH * 100

Also, it means that in your example, 9.11.90 would produce 091190.  I think
it's better if we are able to distinguish 9.11.90 from 9.12.

Also, we should consider doing like Python does, and encode different numbers
in different bytes:

  https://docs.python.org/3/c-api/apiabiversion.html

So we could say ((MAJOR << 16) | (MINOR << 8) | PATCH), for example.  The
advantage with this is that it's easy to to isolate a particular number
using bitshifts and masks.  I know it would be possible as well in decimal
to isolate a particular number, but it's just more convenient in hex.

>  
>  @node Convenience Funs
> diff --git a/gdb/top.c b/gdb/top.c
> index 22e6f7e..97b349a 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -147,22 +147,22 @@ int server_command;
>  
>  /* Timeout limit for response from target.  */
>  
> -/* The default value has been changed many times over the years.  It 
> -   was originally 5 seconds.  But that was thought to be a long time 
> +/* The default value has been changed many times over the years.  It
> +   was originally 5 seconds.  But that was thought to be a long time
>     to sit and wait, so it was changed to 2 seconds.  That was thought
> -   to be plenty unless the connection was going through some terminal 
> +   to be plenty unless the connection was going through some terminal
>     server or multiplexer or other form of hairy serial connection.
>  
> -   In mid-1996, remote_timeout was moved from remote.c to top.c and 
> +   In mid-1996, remote_timeout was moved from remote.c to top.c and
>     it began being used in other remote-* targets.  It appears that the
>     default was changed to 20 seconds at that time, perhaps because the
>     Renesas E7000 ICE didn't always respond in a timely manner.
>  
>     But if 5 seconds is a long time to sit and wait for retransmissions,
> -   20 seconds is far worse.  This demonstrates the difficulty of using 
> +   20 seconds is far worse.  This demonstrates the difficulty of using
>     a single variable for all protocol timeouts.
>  
> -   As remote.c is used much more than remote-e7000.c, it was changed 
> +   As remote.c is used much more than remote-e7000.c, it was changed
>     back to 2 seconds in 1999.  */

Some more unintended changes?  There are more in the patch, I won't point
out all of them.

I have to go for now, I didn't look at the implementation yet.

Simon
  

> Cc: gdb-patches@sourceware.org
> From: Simon Marchi <simark@simark.ca>
> Date: Wed, 20 Mar 2019 21:55:43 -0400
> 
> Ah, I noticed this because you changed the subject in your ping :).

The original one evidently failed to draw attention ;-)

> This would be very useful, especially that we can quite easily query
> this from MI as well as Python.

Thanks, I agree.

> > --- a/gdb/doc/gdb.texinfo
> > +++ b/gdb/doc/gdb.texinfo
> > @@ -11197,7 +11197,7 @@
> >  @vindex $_tlb@r{, convenience variable}
> >  The variable @code{$_tlb} is automatically set when debugging
> >  applications running on MS-Windows in native mode or connected to
> > -gdbserver that supports the @code{qGetTIBAddr} request. 
> > +gdbserver that supports the @code{qGetTIBAddr} request.
> >  @xref{General Query Packets}.
> >  This variable contains the address of the thread information block.
> 
> This looks like an unintended change.

No, it's not unintended.  I mentioned that in my preamble:

> (Note that I took this opportunity to clean up whitespace in top.c,
> I hope it's OK to do that as part of unrelated code changes.)

> I know we plan to move to a version scheme where we don't have a "patch"
> number (a third number), but just in case, maybe we could plan for it anyway
> just in case it ever changes again in the future (I don't expect it will,
> but we never know.
> 
> So something like MAJOR * 10000 + MINOR * 100 + PATCH * 100
> 
> Also, it means that in your example, 9.11.90 would produce 091190.  I think
> it's better if we are able to distinguish 9.11.90 from 9.12.

The idea was that we don't need to distinguish between them.  This
feature is intended to be used in scripts that need to know when a
certain feature became available, so IMO resolution below XX.YY is not
needed, because any XX.YY.ZZ version is just a snapshot of XX.YY+1.

> Also, we should consider doing like Python does, and encode different numbers
> in different bytes:
> 
>   https://docs.python.org/3/c-api/apiabiversion.html
> 
> So we could say ((MAJOR << 16) | (MINOR << 8) | PATCH), for example.  The
> advantage with this is that it's easy to to isolate a particular number
> using bitshifts and masks.  I know it would be possible as well in decimal
> to isolate a particular number, but it's just more convenient in hex.

I don't have a strong opinion, but I do have a weak one: the decimal
"encoding" makes it much easier for humans to construct version
numbers, they don't need to convert to hex.

> > -   As remote.c is used much more than remote-e7000.c, it was changed 
> > +   As remote.c is used much more than remote-e7000.c, it was changed
> >     back to 2 seconds in 1999.  */
> 
> Some more unintended changes?

It's intended, see above.

Thanks for the review.
  

> No, it's not unintended.  I mentioned that in my preamble:

Sorry about that, I see it now.  IMO, you could directly push a patch 
that does these cleanups.

>> (Note that I took this opportunity to clean up whitespace in top.c,
>> I hope it's OK to do that as part of unrelated code changes.)
> 
>> I know we plan to move to a version scheme where we don't have a 
>> "patch"
>> number (a third number), but just in case, maybe we could plan for it 
>> anyway
>> just in case it ever changes again in the future (I don't expect it 
>> will,
>> but we never know.
>> 
>> So something like MAJOR * 10000 + MINOR * 100 + PATCH * 100
>> 
>> Also, it means that in your example, 9.11.90 would produce 091190.  I 
>> think
>> it's better if we are able to distinguish 9.11.90 from 9.12.
> 
> The idea was that we don't need to distinguish between them.  This
> feature is intended to be used in scripts that need to know when a
> certain feature became available, so IMO resolution below XX.YY is not
> needed, because any XX.YY.ZZ version is just a snapshot of XX.YY+1.

Ok, fair enough.  Since the .90 is supposed to behave exactly how the 
next version will (and then never used in production again), I'm fine 
with that.

I'm just a little worried that some day we'll introduce back some 
"patch" number, and then we'll need to change how this number is 
encoded, which will cause some headaches :).

>> Also, we should consider doing like Python does, and encode different 
>> numbers
>> in different bytes:
>> 
>>   https://docs.python.org/3/c-api/apiabiversion.html
>> 
>> So we could say ((MAJOR << 16) | (MINOR << 8) | PATCH), for example.  
>> The
>> advantage with this is that it's easy to to isolate a particular 
>> number
>> using bitshifts and masks.  I know it would be possible as well in 
>> decimal
>> to isolate a particular number, but it's just more convenient in hex.
> 
> I don't have a strong opinion, but I do have a weak one: the decimal
> "encoding" makes it much easier for humans to construct version
> numbers, they don't need to convert to hex.

There's not much difference in hex, you would just write "if 
$_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >= 
901".

But again, my argument for hex is weak too, it's easy enough to extract 
the GDB version components using some "% 100" and "/ 100".  If there's 
no more compelling argument for hex, I'm fine with decimal.

Simon
  

> Date: Thu, 21 Mar 2019 11:02:00 -0400
> From: Simon Marchi <simark@simark.ca>
> Cc: tom@tromey.com, gdb-patches@sourceware.org
> 
> > I don't have a strong opinion, but I do have a weak one: the decimal
> > "encoding" makes it much easier for humans to construct version
> > numbers, they don't need to convert to hex.
> 
> There's not much difference in hex, you would just write "if 
> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >= 
> 901".

That's true for small values of major/minor.  But imagine GDB 12.13
for a moment.

> But again, my argument for hex is weak too, it's easy enough to extract 
> the GDB version components using some "% 100" and "/ 100".  If there's 
> no more compelling argument for hex, I'm fine with decimal.

Maybe someone else have an opinion on this?

Thanks.
  

On 2019-03-21 12:01, Eli Zaretskii wrote:
>> Date: Thu, 21 Mar 2019 11:02:00 -0400
>> From: Simon Marchi <simark@simark.ca>
>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>> 
>> > I don't have a strong opinion, but I do have a weak one: the decimal
>> > "encoding" makes it much easier for humans to construct version
>> > numbers, they don't need to convert to hex.
>> 
>> There's not much difference in hex, you would just write "if
>> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >=
>> 901".
> 
> That's true for small values of major/minor.  But imagine GDB 12.13
> for a moment.

Hmm, true :).

Simon
  

On 03/21/2019 04:06 PM, Simon Marchi wrote:
> On 2019-03-21 12:01, Eli Zaretskii wrote:
>>> Date: Thu, 21 Mar 2019 11:02:00 -0400
>>> From: Simon Marchi <simark@simark.ca>
>>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>>>
>>> > I don't have a strong opinion, but I do have a weak one: the decimal
>>> > "encoding" makes it much easier for humans to construct version
>>> > numbers, they don't need to convert to hex.
>>>
>>> There's not much difference in hex, you would just write "if
>>> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >=
>>> 901".

Is there an advantage of a single variable that encodes the version,
compared to separate major/minor variables?  Separate variables
would render the discussion of how to encode this moot.

GCC has these as predefined preprocessor macros:

 __GNUC__
 __GNUC_MINOR__
 __GNUC_PATCHLEVEL__

We could likewise have separate variables for these.

Thanks,
Pedro Alves
  

On 3/21/19 9:12 AM, Pedro Alves wrote:
> On 03/21/2019 04:06 PM, Simon Marchi wrote:
>> On 2019-03-21 12:01, Eli Zaretskii wrote:
>>>> Date: Thu, 21 Mar 2019 11:02:00 -0400
>>>> From: Simon Marchi <simark@simark.ca>
>>>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>>>>
>>>>> I don't have a strong opinion, but I do have a weak one: the decimal
>>>>> "encoding" makes it much easier for humans to construct version
>>>>> numbers, they don't need to convert to hex.
>>>>
>>>> There's not much difference in hex, you would just write "if
>>>> $_gdb_version >= 0x0901" for example, rather than "if $_gdb_version >=
>>>> 901".
> 
> Is there an advantage of a single variable that encodes the version,
> compared to separate major/minor variables?  Separate variables
> would render the discussion of how to encode this moot.
> 
> GCC has these as predefined preprocessor macros:
> 
>  __GNUC__
>  __GNUC_MINOR__
>  __GNUC_PATCHLEVEL__
> 
> We could likewise have separate variables for these.

I think splitting it out is perhaps the simplest.  FWIW, in FreeBSD we
have a single __FreeBSD_version macro that uses decimal.  We also had
some drama when we originally had a single digit field for the minor
version but had minor versions beyond 9 (4.10, 4.11).  The current
format is:

 * scheme is:  <major><two digit minor>Rxx
 *		'R' is in the range 0 to 4 if this is a release branch or
 *		X.0-CURRENT before releng/X.0 is created, otherwise 'R' is
 *		in the range 5 to 9.
 */
#undef __FreeBSD_version
#define __FreeBSD_version 1300013	/* Master, propagated to newvers */

So release 7.2 would be 702000 and each time a patch is issued to the
release due to an errata or security, the last two digits get bumped
(kind of like a patch number), e.g. 702001, 702002.  The 7.x branch
in between 7.2 and 7.3 is 702500 so it sorts between 7.2 release and
7.3 release.  FreeBSD also bumps the 'patch level' field somewhat
frequently even on 'master' as it's used to denote API (or ABI on
'master') changes.  This has mostly worked out ok, and I do think using
decimal is easier to "read" than hex for this, but split out version
numbers is probably simpler for users to work with.
  

> Cc: tom@tromey.com, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 21 Mar 2019 16:12:26 +0000
> 
> Is there an advantage of a single variable that encodes the version,
> compared to separate major/minor variables?

Just that it makes comparison simpler, and IME less error-prone.  Not
a very important argument, admittedly.
  

On 2019-03-21 1:02 p.m., Eli Zaretskii wrote:
>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>> From: Pedro Alves <palves@redhat.com>
>> Date: Thu, 21 Mar 2019 16:12:26 +0000
>>
>> Is there an advantage of a single variable that encodes the version,
>> compared to separate major/minor variables?
> 
> Just that it makes comparison simpler, and IME less error-prone.  Not
> a very important argument, admittedly.
> 

Yeah, if you want to check if you are running 8.5 or more, you can do:

if $_gdb_version >= 805

With separate components, you have to do something like

if $_gdb_version_major >= 9 \
    || ($_gdb_version_major >= 8 && $gdb_version_minor >= 5

If you need to check for a range, it's even more complicated.

But then, if the version is provided as separate components, nothing 
prevents you to re-encode it however you want after that.  You can do 
easily enough

set $my_gdb_version = ($_gdb_version_major * 100) + $_gdb_version_minor

if you want, and use the simple comparison.  And everyone is free to 
choose their own encoding :).  So, to me separate components sounds better.

Simon
  

On 03/21/2019 06:08 PM, Simon Marchi wrote:
> On 2019-03-21 1:02 p.m., Eli Zaretskii wrote:
>>> Cc: tom@tromey.com, gdb-patches@sourceware.org
>>> From: Pedro Alves <palves@redhat.com>
>>> Date: Thu, 21 Mar 2019 16:12:26 +0000
>>>
>>> Is there an advantage of a single variable that encodes the version,
>>> compared to separate major/minor variables?
>>
>> Just that it makes comparison simpler, and IME less error-prone.  Not
>> a very important argument, admittedly.
>>
> 
> Yeah, if you want to check if you are running 8.5 or more, you can do:
> 
> if $_gdb_version >= 805
> 
> With separate components, you have to do something like
> 
> if $_gdb_version_major >= 9 \
>    || ($_gdb_version_major >= 8 && $gdb_version_minor >= 5
> 
> If you need to check for a range, it's even more complicated.
> 
> But then, if the version is provided as separate components, nothing prevents you to re-encode it however you want after that.  You can do easily enough
> 
> set $my_gdb_version = ($_gdb_version_major * 100) + $_gdb_version_minor
> 
> if you want, and use the simple comparison.  And everyone is free to choose their own encoding :).  So, to me separate components sounds better.

Exactly my thoughts.  That's what the GCC manual describes, BTW:

  https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

Note that with the new gdb version numbering, going forward, for features,
you'll only really care about the major version number, since there won't
be new features added in minor releases.

Thanks,
Pedro Alves
  

> Cc: tom@tromey.com, gdb-patches@sourceware.org
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 21 Mar 2019 18:19:04 +0000
> 
> > if $_gdb_version_major >= 9 \
> >    || ($_gdb_version_major >= 8 && $gdb_version_minor >= 5
> > 
> > If you need to check for a range, it's even more complicated.
> > 
> > But then, if the version is provided as separate components, nothing prevents you to re-encode it however you want after that.  You can do easily enough
> > 
> > set $my_gdb_version = ($_gdb_version_major * 100) + $_gdb_version_minor
> > 
> > if you want, and use the simple comparison.  And everyone is free to choose their own encoding :).  So, to me separate components sounds better.
> 
> Exactly my thoughts.  That's what the GCC manual describes, BTW:
> 
>   https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

And I've seen my share of erroneous version checks using the GCC
macros.

But if there's a consensus to go with 2 separate numbers, I'm okay
with redoing the patch that way.