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

["Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>]

On Tue, Sep 17, 2019 at 3:02 AM Simon Marchi <simark@simark.ca> wrote:
>
> 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.

Thanks for pointing that out, I didn't realize I had that disabled. Fixed now.

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

I wasn't able to build any of them (as "make -C obj/gdb
mips-linux-nat.o, etc.). But I did update what I could. I did also go
through the other files in configure.nat and changed what I could
find.

(note that some of the *-nat.c files are referencing show_debug_regs
which is in a common file, so they didn't need changing)

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

Done. (Also found a place in this file that sets
automatic_hardware_breakpoints to 1, which I 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.

Done.

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

Yeah, I was conflicted on whether to update those variables too;
thanks for pushing me to do it. Done (also required a change in
target.c).

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

Done.

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

Done.

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

Done.

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

Done.

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

I actually changed this file but forgot to commit before running git
format-patch :/ Done.

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

Done.

Will send an updated patch in a second.

Christian