jingham 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);
----------------
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.
================
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:
> 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.
================
Comment at: lldb/bindings/python/python-wrapper.swig:1056
pfunc(ToSWIGWrapper(std::move(debugger)), dict);
----------------
bulbazord wrote:
> 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...
That doesn't seem to be the practice in this file for any of the shared pointer
parameters that we call ToSWIGWrapper on. If we're going to start being more
careful about this, we should do so consistently, which seems outside the scope
of this patch.
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
