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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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..



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:58-61
+  auto utf8 = str.AsUTF8();
+  if (!utf8)
+    return utf8.takeError();
+  return utf8.get();
----------------
lawrence_danna wrote:
> labath wrote:
> > Btw, what are the reasons that this can fail? The string object was created two lines above, so we definitely know it's a string. If the call cannot fail in this particular circumstance, then a one can express that (and make the code simpler) with `return cantFail(str.AsUTF8())`.
> It is possible!
> 
> ```
> chr(0xDFFF).encode('utf8')
> ```
Right. Thanks for explaining.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:864-891
+    auto function = As<PythonCallable>(globals.GetItem("get_arg_info"));
+    if (!function)
+      return function.takeError();
+    get_arg_info = std::move(function.get());
+  }
+
+  Expected<PythonObject> pyarginfo = get_arg_info.Call(*this);
----------------
lawrence_danna wrote:
> labath wrote:
> > Likewise, should most of these `takeErrors` be `cantFail` too ?
> Yea, i guess given that we control the script there's really no reasonable way these can fail if the first check doesn't.
Yes, that's what I was thinking.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:317
+
+#if PY_MAJOR_VERSION < 3
+  static char *py2_const_cast(const char *s) { return const_cast<char *>(s); }
----------------
Could you also add a comment that this is because py2 apis accept strings as "char *", and that one should be careful to not use it if the function can actually modify the string.


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:695
+    EXPECT_EQ(arginfo.get().max_positional_args,
+              PythonCallable::ArgInfo::UNBOUNDED);
+    EXPECT_EQ(arginfo.get().has_varargs, true);
----------------
I don't see this defined anywhere. I guess it leaked from the other patch?


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:700
+  {
+    const char *script = "class Foo: \n"
+                         "  def bar(self, x):\n"
----------------
Raw string too?


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:643-647
+    auto arginfo = lambda.GetArgInfo();
+    ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+    EXPECT_EQ(arginfo.get().count, 1);
+    EXPECT_EQ(arginfo.get().has_varargs, false);
+    EXPECT_EQ(arginfo.get().is_bound_method, false);
----------------
lawrence_danna wrote:
> labath wrote:
> > If you implemented `operator==` and `operator<<(raw_ostream&)` for the ArgInfo class, you could write this as `EXPECT_THAT_EXPECTED(lambda.GetArgInfo(), llvm::HasValue(ArgInfo(1, false, false))`
> eh, do i have to?    I actually like having them each on their own line.   It makes the patches nicer when members are added or removed, and i'd like to remove all the members except max_positional_args.
No, you don't. These are mostly fine. The main thing I don't like is when people then try to "reduce duplication" and create a function like `compare_arginfo`, with the EXPECTs inside. Then, when the checks fail, the error message points you to that function and you're left guessing which of the compare invocations did this fail for. The only thing that can be said here is that if the ASSERT_THAT_EXPECTED macro fails it will terminate the execution of all the subsequent checks too, whereas if this was just one EXPECT call, then you'd be able to see results of all other tests too (and maybe notice a pattern). But that's something I can live with...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68995