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

[Simon Marchi <simark@simark.ca>]

On 4/17/26 7:01 AM, Andrew Burgess wrote:
> 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);
> 
> 
>>    };

Ah, of course.  I initially did not convert
iterate_over_one_compunit_symtab, which is why I had to have the if to
convert from boolean to iteration_status.  I then decided to convert
iterate_over_one_compunit_symtab too, while at it, and left the if in.

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

Thanks, fixed.

The comment also talks about psymtabs, which is stale, but that gets
removed in a subsequent commit IIRC.

>> -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, all fixed locally for now.

Simon