[Lldb-commits] [PATCH] D68995: clean up the implementation of PythonCallable::GetNumArguments

Thu Oct 17 01:07:51 PDT 2019

lawrence_danna marked 3 inline comments as done.
lawrence_danna added a comment.

In D68995#1712387 <https://reviews.llvm.org/D68995#1712387>, @labath wrote:

> In D68995#1711831 <https://reviews.llvm.org/D68995#1711831>, @lawrence_danna wrote:
>
> > In D68995#1710594 <https://reviews.llvm.org/D68995#1710594>, @labath wrote:
> >
> > > Thanks for jumping onto this. Apart from the inline comments, I have one high level question: What are the cases that the old method is wrong for? Was it just builtin functions, or are there other cases too? Is it possible to fix it to work for builtings too? (okay, those were three questions)
> > >
> > > What I am wondering is how important it is to maintain two methods for fetching the argument information. Builtin functions are not likely to be used as lldb callbacks, so if it's just those, it may be sufficient to just leave a TODO to use the new method once we are in a position to require python>=3.3.
> >
> >
> > Looks like the old implementation also doesn't work for class or static methods, or for objects with `__call__` method.    I added a bunch of tests for script commands with various callable types in the followon patch.
>
>
> Ok, I see... Would it be hard/impossible to fix that using the previous method, so that we have a unified implementation? Or would that result in a bunch of ifdefs too ? I'm sorry if that's obvious, but I don't know much about the C python api..

I did fix the previous method for python2, at least for the callable types I could think of to test.

There must be some way to get something resembling the old method to work for python3, because `inspect` does it.   But it would be harder to get right, and a lot less likely to keep being correct in future versions of python.    And I don't think it's possible to have a single implementation like that that works for both 2 and 3 without ifdefs.

I was thinking that since python2 is EOL in like two months anyway, the best thing to do is to just keep the old implementation around until we drop python 2 support, and then delete it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68995