jingham added inline comments.
================
Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:315
+ if (filter_by_cu && filter_by_function) {
+ // Keep this symbol context if it is a function call to a function
+ // whose declaration is located in a file that passes. This is needed
----------------
labath wrote:
> kwk wrote:
> > labath wrote:
> > > This mention of a "function call" is confusing. Either a function is in a
> > > file or it isn't. Why do we care about who calls who?
> > @labath we have this `filter_by_function` as an indirection to let
> > `BreakpointResolverName::SearchCallback` know that the `SearchFilter` needs
> > a special handling. But while reviewing the code now I noticed that only
> > the implementations of `SearchFilter::GetFilterRequiredItems` currently
> > either return
> >
> > - `eSymbolContextModule` (for `SearchFilterByModule` and
> > `SearchFilterByModuleList`) or
> > - `eSymbolContextModule | eSymbolContextCompUnit |
> > eSymbolContextFunction` (for `SearchFilterByModuleListAndCU`).
> >
> > I thought this was clever at a time when the old
> > `SearchFilterByModuleListAndCU` which then only returned
> > `eSymbolContextModule | eSymbolContextCompUnit `. With my new search filter
> > I wanted a special handling which is why I wrote a new search filter class
> > and had it return `eSymbolContextModule | eSymbolContextCompUnit |
> > eSymbolContextFunction`. But when you confirmed my question to change the
> > default behavior (https://reviews.llvm.org/D74136#1869622), the new class
> > became the default implementation and there no longer is a need to
> > distinguish the required items for this search filter.
> >
> > Makes sense?
> I'm afraid I got a bit lost here. Jim is more familiar with his stuff, so
> maybe he can answer this question. Jim?
>
> Looking at this again, I get the impression that a lot of this confusion is
> coming from the fact that this patch assigns a special meaning of
> `eSymbolContextCompUnit | eSymbolContextFunction` combination to mean "match
> the file the function is defined in". I think it would be more consistent
> with the spirit of these functions if this code just did something like:
> ```
> if (filter_by_function && !filter.FunctionPasses(function))
> remove_it = true;
> ```
> and then the code which extracts the file from the function definition lived
> inside the `FunctionPasses` method. That may require adding a new filter
> class or it could be that an existing filter could be adapted to do that
> (SearchFilterByModuleListAndCU, I guess).
>
> Take this with a grain of salt though, as I don't frequent this part of code
> very often.
I think Pavel is right here.
The point of the built-in checks: ModulePasses, CompUnitPasses, FunctionPasses
is that they mirror the search space that the Searcher iterates over when
calling the Resolvers. It allows a Resolver to have the Searcher bypass
calling the resolver at all. Adding in the DeclFile to the CompUnit check
isn't right for that use, since that isn't one of the spaces the Searcher
iterates over.
The way I thought about this is that all the higher level checks are for the
searcher, but ultimately you always have to call AddressPasses because your
filter might be below the chunks that the Searcher searches for.
So it seems to me better not to try to glom the CompUnit check and the DeclFile
check together, but rather to have the CompUnit check do what it says,
eliminate comp units. And then add the DeclFile check to the AddressPasses for
your filter.
It also seems a shame that if I really only want to look only in a CompUnit, I
end up having to call into the Resolver for every comp unit, and then filter
there.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74136/new/
https://reviews.llvm.org/D74136
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
