[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added inline comments.


================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:469
+  
+  virtual int GetNumArgumentsForCallable(const char *callable_name) { 
+    return -1;
----------------
labath 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.
> 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.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68671