jingham marked 2 inline comments as done.
jingham added inline comments.
================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+
+ virtual int GetNumArgumentsForCallable(const char *callable_name) {
+ return -1;
----------------
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.
================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+
+ virtual int GetNumArgumentsForCallable(const char *callable_name) {
+ return -1;
----------------
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.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68671/new/
https://reviews.llvm.org/D68671
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
