labath added a comment.
At this point I'm getting very lost in this patch, but here's some of my
thoughs (the ones I could actually formulate in a coherent manner)...
================
Comment at: lldb/source/Core/SearchFilter.cpp:713
+ if (!type)
+ return SearchFilterByModuleList::FunctionPasses(function);
+
----------------
kwk wrote:
> jankratochvil wrote:
> > If we cannot determine which file the function is from then rather ignore
> > it (the current call returns `true`):
> > ```
> > return false;
> > ```
> @jankratochvil so far I haven't addressed this on purpose to give @labath and
> @jingham time to say what they think about this.
Yes, ignoring the function if we cannot determine which file it is in sounds
reasonable. (though I'm not sure if this type argument can ever be null in
practice.)
================
Comment at:
lldb/test/API/functionalities/breakpoint/scripted_bkpt/TestScriptedResolver.py:124
- file_list.Append(lldb.SBFileSpec("noFileOfThisName.xxx"))
- wrong.append(target.BreakpointCreateFromScript("resolver.Resolver",
extra_args, module_list, file_list))
-
----------------
kwk wrote:
> @jingham can you please let me know if this test serves any purpose with my
> patch? I mentioned in my last change log
> (https://reviews.llvm.org/D74136#2113899) that it calls `AddressPasses` and
> checked for the CU before. But since we're no longer doing this, this test no
> longer can pass.
After reading more about it, I'm pretty sure this is not the right solution.
The scripted breakpoints are documented ([[
https://github.com/llvm/llvm-project/blame/master/lldb/docs/use/python-reference.rst#L390
| here ]]) as accepting filters based on compile units. If we don't want to
change that, then I guess we need to create a separate filter, so that the
scripted breakpoints can filter based on compile unit names and the name+file
breakpoints can filter on definitions file names.
If we want to change that, then we'd also need to update the documentation.
But, the test should stay anyway, because `noFileOfThisName.xxx` is not a valid
file nor compile unit name, so the location should not be added regardless of
what exactly are we choosing to filter on.
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
