Re: [PATCH 09/11] gdb: change objfile::map_symtabs_matching_filename to find_symtab_matching_filename

[Simon Marchi <simon.marchi@efficios.com>]

On 4/17/26 9:50 AM, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> The only user of objfile::map_symtabs_matching_filename uses that method
>> to find the first matching symtab.  It would therefore be more natural
>> for that method to be a "find" method, returning the first symtab
>> matching the predicate.
>>
>> Change map_symtabs_matching_filename to be
>> find_symtab_in_compunit_symtab, and the internal
>> iterate_over_one_compunit_symtab to be find_symtab_in_compunit_symtab.
> 
> Typo:
> 
>   Change map_symtabs_matching_filename to be find_symtab_in_compunit_symtab, and ..." 
> 
> Should be:
> 
>   Change map_symtabs_matching_filename to be find_symtab_matching_filename, and ..." 

Done.

>>
>> This makes function find_symtab simpler.
>>
>> Change-Id: Id14a95498fad243495d6eab18810d0c4ab8dbf90
>> ---
>>  gdb/objfiles.h      | 16 +++-----
>>  gdb/symfile-debug.c | 89 +++++++++++++++++++++------------------------
>>  gdb/symtab.c        | 20 ++--------
>>  3 files changed, 52 insertions(+), 73 deletions(-)
>>
> 
>> diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c
>> index 8e4892649508..d90f400f379c 100644
>> --- a/gdb/symfile-debug.c
>> +++ b/gdb/symfile-debug.c
>> @@ -159,29 +159,30 @@ objfile::forget_cached_source_info ()
>>      iter->forget_cached_source_info (this);
>>  }
>>  
>> -/* Check for a symtab of a specific name by searching some symtabs.
>> +/* Find the first symtab of CUST matching BASE_NAME, NAME and REAL_PATH, for
>> +   which CALLBACK returns true.
>>  
>>     If NAME is not absolute, then REAL_PATH is NULL
>>     If NAME is absolute, then REAL_PATH is the gdb_realpath form of NAME.
>>  
>>     The return value, NAME, REAL_PATH and CALLBACK are identical to the
>> -   `map_symtabs_matching_filename' method of quick_symbol_functions.
>> +   `find_symtab_matching_filename' method of quick_symbol_functions.
> 
> This comment doesn't make sense.  The find_symtab_matching_filename
> function is a member function of `objfile` not
> `quick_symbol_functions`.  You didn't write that, but it looks like a
> pre-existing mistake in the comment.  Can you fix that please?  Maybe:
> 
>    The return value, NAME, REAL_PATH and CALLBACK are identical to the
>    `objfile::find_symtab_matching_filename' method.
> 
> Would do?

Ah, it's possible that
quick_symbol_functions::map_symtabs_matching_filename existed before,
and then it was merged into the do-it-all search method.

Fixed as you suggested.

>> @@ -254,6 +258,7 @@ objfile::map_symtabs_matching_filename
>>      return false;
>>    };
>>  
>> +  symtab *result = nullptr;
>>    auto compunit_callback = [&] (compunit_symtab *symtab)
>>    {
>>      /* Skip included compunits, as they are searched by
> 
> The next line of this comment references
> iterate_over_one_compunit_symtab, and should be updated to
> find_symtab_in_compunit_symtab.

Fixed.

>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index 6a5c8c7058e4..fd841318387e 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -637,8 +637,6 @@ static symtab *
>>  find_symtab (program_space *pspace, const char *name,
>>  	     find_symtab_callback_ftype callback)
>>  {
>> -  struct symtab *result = NULL;
>> -
> 
> Ahh, I should have looked ahead!  I complained about this in the last
> patch, but I see it's now gone.  No need to change the last patch I
> guess.

No worries, it was easy to fix.  I also don't look ahead much when
reviewing patches, otherwise it's a lot of tokens to hold in the small
context window in my brain.

> 
>>    gdb::unique_xmalloc_ptr<char> real_path;
>>  
>>    /* Here we are interested in canonicalizing an absolute path, not
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon