Re: [PATCH 03/11] gdb: introduce iteration_status enum, use it for search callbacks

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simon.marchi@efficios.com> writes:

> There are a bunch of iteration functions that take a callback returning
> true or false to indicate whether to continue or stop iterating.  These
> functions then return the same value, indicate whether the iteration was
> done until the end of interrupted.  I think this is confusing and
> error-prone, as I never know which value means what.  It is especially
> confusing when two opposite conventions collide, such as in
> objfile::map_symtabs_matching_filename.
>
> I propose to make that more obvious by introducing a new
> iteration_status enum with self-documenting values.
>
> I started to change the callback type
> compunit_symtab_iteration_callback, taken by
> quick_symbol_functions::search, and then followed that path to update a
> bunch of other functions.
>
> I chose the name to be kind of generic, so that it can be used for other
> similar iteration patterns.  I also put it in gdbsupport, in case we
> want to use it in gdbserver too.
>
> Change-Id: I55d84d0c1af8ac0b82cc9f49ccf0d6b60e1769e0


> @@ -260,30 +259,37 @@ objfile::map_symtabs_matching_filename
>      /* Skip included compunits, as they are searched by
>         iterate_over_one_compunit_symtab.  */
>      if (symtab->user != nullptr)
> -      return true;
> +      return iteration_status::keep_going;
>  
> -    /* CALLBACK returns false to keep going and true to continue, so
> -       we have to invert the result here, for search.  */
> -    return !iterate_over_one_compunit_symtab (name_basename, name, real_path,
> -					      symtab, callback);
> +    /* iterate_over_one_compunit_symtab returns true to stop,
> +       convert to iteration_status.  */
> +    if (iterate_over_one_compunit_symtab (name_basename, name, real_path,
> +					  symtab, callback)
> +	== iteration_status::stop)
> +      return iteration_status::stop;
> +
> +    return iteration_status::keep_going;

I think this needs fixing.  You've updated
iterate_over_one_compunit_symtab to return an iteration_status, so I
think you can just do:

  return iterate_over_one_compunit_symtab (name_basename, name, real_path,
					   symtab, callback);


>    };



> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index 0db85bcf5baa..9e45f087adda 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -30,6 +30,7 @@
>  #include "gdbsupport/gdb_regex.h"
>  #include "gdbsupport/enum-flags.h"
>  #include "gdbsupport/function-view.h"
> +#include "gdbsupport/iteration-status.h"
>  #include <optional>
>  #include <string_view>
>  #include "gdbsupport/next-iterator.h"
> @@ -2795,8 +2796,9 @@ bool compare_filenames_for_search (const char *filename,
>     Call CALLBACK with each symtab that is found.  If CALLBACK returns
>     true, the search stops.  */

This comment that's just peeking in here, needs to be updated.  CALLBACK
no longer returns a bool.

>  
> -void iterate_over_symtabs (program_space *pspace, const char *name,
> -			   gdb::function_view<bool (symtab *)> callback);
> +void iterate_over_symtabs
> +  (program_space *pspace, const char *name,
> +   gdb::function_view<iteration_status (symtab *)> callback);

With these fixed, looks good.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew