Re: [PATCH v2] Change boolean options to bool instead of int

[Simon Marchi <simark at simark dot ca>]

On 2019-09-15 11:10 p.m., Christian Biesinger via gdb-patches wrote:
> This is for add_setshow_boolean_cmd as well as the gdb::option interface.

Hi Christian,

I get this error when building, which probably means you are not building with Guile support:

/home/smarchi/src/binutils-gdb/gdb/guile/scm-auto-load.c: In function ‘void gdbscm_initialize_auto_load()’:
/home/smarchi/src/binutils-gdb/gdb/guile/scm-auto-load.c:73:36: error: cannot convert ‘int*’ to ‘bool*’ for argument ‘3’ to ‘cmd_list_element* add_setshow_boolean_cmd(const char*, command_class, bool*, const char*, const char*, const char*, void (*)(const char*, int, cmd_list_element*), void (*)(ui_file*, int, cmd_list_element*, const char*), cmd_list_element**, cmd_list_element**)’
       auto_load_show_cmdlist_get ());
                                    ^

FYI, I think we don't support Guild 2.2 at the moment, so I am using --with-guile=guile-2.0 when configuring.

This also reminded me that there are some uses of add_setshow_boolean_cmd in native files.
Doing a

  grep add_setshow_boolean_cmd *-nat.c

to find them would be a good start, but actually all the files listed in configure.nat should
be considered, as they may not be included in your build.

For those native files, you can try to build test some if you want to, but you probably won't
be able to build all of them.  That is fine, just do a best effort of changing the spots you
find.

> @@ -350,7 +350,7 @@ show_pending_break_support (struct ui_file *file, int from_tty,
>     set with "break" but falling in read-only memory.
>     If 0, gdb will warn about such breakpoints, but won't automatically
>     use hardware breakpoints.  */
> -static int automatic_hardware_breakpoints;
> +static bool automatic_hardware_breakpoints;

The comment above this one should be updated.

> diff --git a/gdb/dwarf-index-cache.c b/gdb/dwarf-index-cache.c
> index e56cb59343c..74f06dfbfb7 100644
> --- a/gdb/dwarf-index-cache.c
> +++ b/gdb/dwarf-index-cache.c
> @@ -33,7 +33,7 @@
>  #include <stdlib.h>
>  
>  /* When set to 1, show debug messages about the index cache.  */
> -static int debug_index_cache = 0;
> +static bool debug_index_cache = false;

The comment should be updated.

> @@ -206,7 +206,7 @@ set_disable_randomization (const char *args, int from_tty,
>  /* User interface for non-stop mode.  */
>  
>  int non_stop = 0;
> -static int non_stop_1 = 0;
> +static bool non_stop_1 = false;

I'd suggest making non_stop a bool as well, unless you judge that doing that has too many
fallouts.  But I think it will be fine, as it just seems to be used everywhere as a boolean
anyway.

>  
>  static void
>  set_non_stop (const char *args, int from_tty,
> @@ -214,7 +214,7 @@ set_non_stop (const char *args, int from_tty,
>  {
>    if (target_has_execution)
>      {
> -      non_stop_1 = non_stop;
> +      non_stop_1 = (non_stop != 0);
>        error (_("Cannot change this setting while the inferior is running."));
>      }
>  
> @@ -235,7 +235,7 @@ show_non_stop (struct ui_file *file, int from_tty,
>     target's execution have been disabled.  */
>  
>  int observer_mode = 0;
> -static int observer_mode_1 = 0;
> +static bool observer_mode_1 = false;

Same here.

> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -105,7 +105,7 @@ static int mi_async = 0;
>  
>  /* The set command writes to this variable.  If the inferior is
>     executing, mi_async is *not* updated.  */
> -static int mi_async_1 = 0;
> +static bool mi_async_1 = false;
>  
>  static void
>  set_mi_async_command (const char *args, int from_tty,
> @@ -113,7 +113,7 @@ set_mi_async_command (const char *args, int from_tty,
>  {
>    if (have_live_inferiors ())
>      {
> -      mi_async_1 = mi_async;
> +      mi_async_1 = (mi_async != 0);

Instead of doing this, I think you could just make mi_async a bool as well.

> diff --git a/gdb/symfile.c b/gdb/symfile.c
> index 3cd514409b0..2792ab43282 100644
> --- a/gdb/symfile.c
> +++ b/gdb/symfile.c
> @@ -151,7 +151,7 @@ static const char *print_symbol_loading = print_symbol_loading_full;
>     library symbols are not loaded, commands like "info fun" will *not*
>     report all the functions that are actually present.  */
>  
> -int auto_solib_add = 1;
> +bool auto_solib_add = true;

This comment above this one should be updated.  Or really, it should be changed
to

  /* See symfile.h.  */

as it's not static, and we have the exact same comment in symfile.h already.  If you
want to make this change in an obvious patch before this one, I'd be ideal.

> --- a/gdb/symfile.h
> +++ b/gdb/symfile.h
> @@ -449,7 +449,7 @@ extern section_addr_info
>     library symbols are not loaded, commands like "info fun" will *not*
>     report all the functions that are actually present.  */
>  
> -extern int auto_solib_add;
> +extern bool auto_solib_add;

This comment above this one should be updated.

> @@ -3853,7 +3853,7 @@ maint_set_target_async_command (const char *args, int from_tty,
>  {
>    if (have_live_inferiors ())
>      {
> -      target_async_permitted_1 = target_async_permitted;
> +      target_async_permitted_1 = (target_async_permitted != 0);

You've already changed target_async_permitted to a bool (which I think is good), so you
don't need to change this line.

>        error (_("Cannot change this setting while the inferior is running."));
>      }
>  
> @@ -3933,24 +3933,24 @@ maint_show_target_non_stop_command (struct ui_file *file, int from_tty,
>  
>  /* Temporary copies of permission settings.  */
>  
> -static int may_write_registers_1 = 1;
> -static int may_write_memory_1 = 1;
> -static int may_insert_breakpoints_1 = 1;
> -static int may_insert_tracepoints_1 = 1;
> -static int may_insert_fast_tracepoints_1 = 1;
> -static int may_stop_1 = 1;
> +static bool may_write_registers_1 = true;
> +static bool may_write_memory_1 = true;
> +static bool may_insert_breakpoints_1 = true;
> +static bool may_insert_tracepoints_1 = true;
> +static bool may_insert_fast_tracepoints_1 = true;
> +static bool may_stop_1 = true;
>  
>  /* Make the user-set values match the real values again.  */
>  
>  void
>  update_target_permissions (void)
>  {
> -  may_write_registers_1 = may_write_registers;
> -  may_write_memory_1 = may_write_memory;
> -  may_insert_breakpoints_1 = may_insert_breakpoints;
> -  may_insert_tracepoints_1 = may_insert_tracepoints;
> -  may_insert_fast_tracepoints_1 = may_insert_fast_tracepoints;
> -  may_stop_1 = may_stop;
> +  may_write_registers_1 = (may_write_registers != 0);
> +  may_write_memory_1 = (may_write_memory != 0);
> +  may_insert_breakpoints_1 = (may_insert_breakpoints != 0);
> +  may_insert_tracepoints_1 = (may_insert_tracepoints != 0);
> +  may_insert_fast_tracepoints_1 = (may_insert_fast_tracepoints != 0);
> +  may_stop_1 = (may_stop != 0);

Suggest changing all those variables on the right hand side to be bool
at the same time, so you won't need this change.

Simon