[Lldb-commits] [PATCH] D146152: Add __lldb_init_module_with_target for use when sourcing modules for a target

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
Comment at: lldb/bindings/python/python-wrapper.swig:1031-1048
+  // First call the target initializer:
+  if (target) {
+    python_function_name_string += ".__lldb_init_module_with_target";
+    python_function_name = python_function_name_string.c_str();
+
+    auto pfunc = PythonObject::ResolveNameWithDictionary<PythonCallable>(
+      python_function_name, dict);
----------------
JDevlieghere wrote:
> I'm surprised we might call both. The way I expected this to work is that if `__lldb_init_module_with_target` is defined, that's what we use if wee have a target and otherwise fall back to `__lldb_init_module` assuming it's defined.
> 
> What's the benefit of calling both? Do you expect the implementation to be different? Or do you think it's more likely that the implementations will be similar, just one having access to the target? 
I can see the advantage to calling both: The use of `__lldb_init_module_with_target` can be used to set up things specific to your target and `__lldb_init_module` can be used for more general things. That being said, you should be able to do everything with `__lldb_init_module_with_target` since if you have a target, you should have a debugger too.

Whatever we choose to go with, the behavior should be explicitly documented here: https://lldb.llvm.org/use/python-reference.html (`llvm-project/lldb/docs/use/python-reference.rst`). We already document one, we should be documenting this one and the expected behavior when both are present.


================
Comment at: lldb/bindings/python/python-wrapper.swig:1056
 
   pfunc(ToSWIGWrapper(std::move(debugger)), dict);
 
----------------
I know you didn't touch this but we're assuming debugger isn't nullptr here right? Should we add an assert? I know we'd have to be in a bad state to get to this point but an assertion may help catch that kind of thing if we do hit this point...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146152/new/

https://reviews.llvm.org/D146152