labath 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
----------------
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.
================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:15
+# CHECK: (lldb) breakpoint set -n inlined_42
+# CHECK-NEXT: Breakpoint 1: where = dummy.out`inlined_42() + 4 at
search-support-files.h:1:20, address = 0x0{{.*}}
+
----------------
kwk wrote:
> labath wrote:
> > These check lines hardcode too much stuff. The `+4` thingy can easily
> > change due to unrelated codegen changes, and even the `:1:20` seems
> > unnecessarily strict.
> > Maybe something like this would be enough:
> > ```
> > CHECK-NEXT: Breakpoint 1: where = {{.8}}`inlined_42{{.*}} at
> > search-support-files.h
> > ```
> Yes, you're right. Although I don't know what `{{.8}}` does in this case.
I meant `{{.*}}`.
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
