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);
----------------
jingham wrote:
> jingham wrote:
> > bulbazord wrote:
> > > 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.
> > How you would use this depends on how you expect your python module to be 
> > used.  
> > 
> > 1) If your python module is only going to be sourced from a Scripting 
> > Resource in a Module, then you could just implement 
> > `__lldb_init_module_with_target`.  
> > 
> > 2) If your python module is imported through `command script import`, then 
> > it will have to implement `__lldb_init_module` since it's really never 
> > initialized in the presence of a specific target, so I wouldn't have a 
> > valid target to pass in.  I don't want to pass in something like the 
> > "currently selected target" in this case just so there's something you can 
> > get a debugger from, that kinda lying...
> > 
> > 3) It's only if you want this python module to be used in either mode, or 
> > if you need to support lldb's that have and don't have this new API, that 
> > you need to have both API's.  In that case, it seemed more useful to let 
> > the user separate the "I'm being initialized as a particular target's 
> > scripting resource" part of the initialization from the more general part.
> I will add docs once we've decided the right way to do this.
I feel like this may lead to some confusion. `__lldb_init_module_with_target` 
makes sense as the thing that runs when you source the python file from dSYM 
and `__lldb_init_module` makes sense as the thing that runs when you do 
`command script import`. Implicitly running `__lldb_init_module` when you are 
imported as a scripting resource in a module would mean there's no way to get 
different behavior depending on which thing ran. Specifically, 
`__lldb_init_module` is always guaranteed to run. If a user wanted both to run, 
I think it might be better as an explicit thing. Like 
`__lldb_init_module_with_target` can call `__lldb_init_module`... I'm not sure, 
what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146152

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to