[Lldb-commits] [lldb] Remove redundant symbol lookups in IRExecutionUnit::FindInSymbols (PR #102835)

[Augusto Noronha via lldb-commits]

augusto2112 wrote:

> > > > Oh, wait a sec. I actually changed the behavior of the `IRExecutionUnit::FindInSymbols`. It used to exit early if the function was found in `module_sp`, but now we will always scan through the whole ModuleList. And we can't change the behavior of the `ModuleList::FindFunctions` to return after the first match, because it might be not what we want in general case. Maybe I should add more generic versions of these functions taking a callback to invoke on each match instead of SymbolContextList? Something like
> > > > ```
> > > > void ModuleList::FindSymbolsWithNameAndType(
> > > >   ConstString name,
> > > >   lldb::SymbolType symbol_type,
> > > >   const SymbolContext *search_hint,
> > > >   llvm::function_ref<bool(const Symbol&)> callback
> > > >   )
> > > > ```
> > > > 
> > > > 
> > > >     
> > > >       
> > > >     
> > > > 
> > > >       
> > > >     
> > > > 
> > > >     
> > > >   
> > > > where the result of `callback` indicates whether the search should be continued or not.
> > > 
> > > 
> > > I think it'd be fine to return early if the type if found with the `search_hint`, as long as this is clearly documented in the API. My impression is that this would be a more useful behavior than just reordering the search results so the `search_hint` results are first, and I don't think we need the extra flexibility provided by that callback at the moment. I don't touch this code very often though so if @clayborg or @Michael137 disagree I'd defer to them.
> > 
> > 
> > Probably fine returning early unconditionally tbh, but only if the `search_hint` is specified. It shouldn't break anything?
> 
> It wouldn't be that good API then, because the function takes an optional argument that changes the behavior significantly:
> 
> * when it's provided the function returns a single result(which means we don't really need `SymbolContextList`)
> * when it's NULL, the function will find all the matches



> > > > Oh, wait a sec. I actually changed the behavior of the `IRExecutionUnit::FindInSymbols`. It used to exit early if the function was found in `module_sp`, but now we will always scan through the whole ModuleList. And we can't change the behavior of the `ModuleList::FindFunctions` to return after the first match, because it might be not what we want in general case. Maybe I should add more generic versions of these functions taking a callback to invoke on each match instead of SymbolContextList? Something like
> > > > ```
> > > > void ModuleList::FindSymbolsWithNameAndType(
> > > >   ConstString name,
> > > >   lldb::SymbolType symbol_type,
> > > >   const SymbolContext *search_hint,
> > > >   llvm::function_ref<bool(const Symbol&)> callback
> > > >   )
> > > > ```
> > > > 
> > > > 
> > > >     
> > > >       
> > > >     
> > > > 
> > > >       
> > > >     
> > > > 
> > > >     
> > > >   
> > > > where the result of `callback` indicates whether the search should be continued or not.
> > > 
> > > 
> > > I think it'd be fine to return early if the type if found with the `search_hint`, as long as this is clearly documented in the API. My impression is that this would be a more useful behavior than just reordering the search results so the `search_hint` results are first, and I don't think we need the extra flexibility provided by that callback at the moment. I don't touch this code very often though so if @clayborg or @Michael137 disagree I'd defer to them.
> > 
> > 
> > Probably fine returning early unconditionally tbh, but only if the `search_hint` is specified. It shouldn't break anything?
> 
> It wouldn't be that good API then, because the function takes an optional argument that changes the behavior significantly:
> 
> * when it's provided the function returns a single result(which means we don't really need `SymbolContextList`)
> * when it's NULL, the function will find all the matches

Then you probably want to implement something similar to `FindTypes` which takes in a `TypeQuery`, like @Michael137 said.

https://github.com/llvm/llvm-project/pull/102835