lawrence_danna added inline comments.
================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+
+ virtual int GetNumArgumentsForCallable(const char *callable_name) {
+ return -1;
----------------
jingham wrote:
> lawrence_danna wrote:
> > labath wrote:
> > > jingham wrote:
> > > > lawrence_danna wrote:
> > > > > labath wrote:
> > > > > > In light of varargs functions (`*args, **kwargs`), which are fairly
> > > > > > popular in python, the concept of "number of arguments of a
> > > > > > callable" does not seem that well defined. The current
> > > > > > implementation seems to return the number of fixed arguments, which
> > > > > > might be fine, but I think this behavior should be documented.
> > > > > > Also, it would be good to modernize this function signature -- have
> > > > > > it take a StringRef, and return a `Expected<unsigned (?)>` --
> > > > > > ongoing work by @lawrence_danna will make it possible to return
> > > > > > errors from the python interpreter, and this will make it possible
> > > > > > to display those, instead of just guessing that this is because the
> > > > > > callable was not found (it could in fact be because the named thing
> > > > > > is not a callable, of because resolving the name produced an
> > > > > > exception, ...).
> > > > > I just took a look at PythonCallable::GetNumArguments() and it's
> > > > > horribly broken.
> > > > >
> > > > > It doesn't even work for the simplest test case I could think of.
> > > > >
> > > > > ```
> > > > > auto builtins = PythonModule::Import("builtins");
> > > > > ASSERT_THAT_EXPECTED(builtins, llvm::Succeeded());
> > > > > auto hex = As<PythonCallable>(builtins.get().GetAttribute("hex"));
> > > > > ASSERT_THAT_EXPECTED(hex, llvm::Succeeded());
> > > > > EXPECT_EQ(hex.get().GetNumArguments().count, 1u);
> > > > > ```
> > > > >
> > > > > we should really re-write it to use inspect.signature.
> > > > Interesting. We use it for free functions (what you pass to the -F
> > > > option of `breakpoint command add`) and for the __init__ and __call__
> > > > method in the little classes you can make up for scripted thread plans
> > > > and for the class version of Python implemented command-line commands.
> > > > We have tests for telling 3 vrs. 4 vrs. not enough or too many, and
> > > > they all pass. So it seems to work in the cases we currently need it
> > > > to work for...
> > > >
> > > > "inspect.signature" is python 3 only, and the python 2 equivalent is
> > > > deprecated. So it will take a little fancy footwork to use it in the
> > > > transition period.
> > > lol :)
> > >
> > > I would actually say that we should try not to use this function(ality)
> > > wherever possible. Making decisions based on the number of arguments the
> > > thing you're about to call takes sounds weird. I don't want to get too
> > > involved in this, but I was designing this, I'd just say that if one
> > > tries to pass arguments to the callback then the callback MUST take three
> > > arguments (or we'll abort processing the breakpoint command). If he wants
> > > his function to be called both with arguments and without, he can just
> > > add a default value to the third argument. (And if his function takes two
> > > arguments, but he still tries to pass something... well, it's his own
> > > fault).
> > >
> > > Anyway, feel free to ignore this comment, but I felt like I had to say
> > > something. :)
> > I completely agree with Pavel. Inspecting a function signature before
> > calling it is a big code smell in python. If there's a way to avoid
> > doing that introspection, that would be better.
> Unfortunately, we originally designed this interface to take three arguments,
> the frame pointer, the breakpoint location and the Python session dict. Then
> it became clear that it would be better to add this extra args argument (and
> in the case of Python based commands the ExecutionContext pointer). At that
> point we had three choices, abandon the improvement; switch to
> unconditionally passing the extra arguments, and break everybody's extant
> uses; or switch off of the number of arguments to decide whether the user had
> provided the old or new forms.
>
> My feeling about lldb Python scripts/commands etc. that people use in the
> debugger is that a lot of users don't know how they work at all, they just
> got them from somebody else; and many more figured out how to write them for
> some specific purpose, and then pretty much forgot how they worked. So
> suddenly breaking all these bits of functionality will result in folks just
> deciding that this affordance is not reliable and not worth their time, which
> would be a shame.
>
> So instead we accommodate both forms, which requires that we know which one
> the user provided. If you see a better way to do this, (and are willing to
> implement it because so far as I can see this method is going to work just
> fine) dig in, I'm not wedded to the particular approach. But I am not
> excited about penalizing our users because we didn't get the API design right
> the first time through.
makes sense.
The only other way I can think of to solve it would be to have some indication
in the `break com add` command of what signature it expects from the function.
But that's really ugly too because now you're asking users to understand yet
another option.
I put up https://reviews.llvm.org/D68995 this morning which adds
`inspect.signature` support for pythons that have it.
Currently we have really gnarly ArgInfo tests like this:
```
if (argc.count == 5 || argc.is_bound_method || argc.has_varargs)
pfunc(debugger_arg, PythonString(args), exe_ctx_arg, cmd_retobj_arg,
dict);
else
pfunc(debugger_arg, PythonString(args), cmd_retobj_arg, dict);
```
😖
I think if we replace `count` with `max_positional_args` we should be able to
replace that kindof test with just
```
if (argc.max_positional_args < 5)
old_version
else
new_version
```
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68671/new/
https://reviews.llvm.org/D68671
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
