labath added a comment.
In D74136#1983467 <https://reviews.llvm.org/D74136#1983467>, @kwk wrote:
> I'd be happy to hear about your comments @labath because I'm kind of stuck in
> what I can think of. I will obviously rename `SearchFilterByModuleListAndCU`
> but that can come in a child revision.
Sorry, I'm kinda busy these days, so and I did not see the embedded question
while glossing over the WIP changes.
I think this looks pretty clean (surprisingly clean even) now. I don't have any
major comments, but we should have Jim take a look at this again, as he's more
familiar with this stuff.
Given that the name `SearchFilterByModuleListAndCU` is not very widespread (20
hits in three files) and that this patch actually makes that name wrong, I
think it'd be best to do the rename as a part of this patch.
================
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
----------------
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?
================
Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:321
+ FileSpec &source_file = decl.GetFile();
+ if (!filter.CompUnitPasses(source_file))
+ remove_it = true;
----------------
This `CompUnitPasses` call smells, as `source_file` does not really correspond
to any compilation unit (in the scenario that you care about). It seems like
this is actually the first usage of this function, so let's just rename it (and
make it take a `const FileSpec &` while we're at it).
================
Comment at: lldb/test/Shell/Breakpoint/Inputs/search-support-files.h:1
+int inlined_42() { return 42; }
----------------
Calling this `inlined` is misleading. The function won't get inlined anywhere
at -O0, and in fact your test would not work if it got inlined. Maybe just call
it `function_in_header` ?
================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:8
+
+# RUN: %build %p/Inputs/search-support-files.cpp -o dummy.out
+# RUN: %lldb -b -s %s dummy.out | FileCheck --color --dump-input=fail %s
----------------
All of the tests execute in the same directory, so it's a good practice to
embed `%t` into the files you create. As you don't care about the actual file
name in the CHECK lines, you can just replace dummy.out with `{{.*}}`
everywhere.
================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:9
+# RUN: %build %p/Inputs/search-support-files.cpp -o dummy.out
+# RUN: %lldb -b -s %s dummy.out | FileCheck --color --dump-input=fail %s
+
----------------
It's not standard/polite to hard-code --color like this. Using --dump-input is
also not very common though there are definitely precedents for that, and I can
see how it can be useful (though one should be careful not to overwhelm the
terminal if he's using it).
================
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{{.*}}
+
----------------
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
```
================
Comment at: lldb/test/Shell/Breakpoint/search-support-files.test:32
+# NOTE: This test is the really interesting one as it shows that we can
+# search by source files that are themselves no compulation units.
+
----------------
compilation
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74136/new/
https://reviews.llvm.org/D74136
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
