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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:58-61
+  auto utf8 = str.AsUTF8();
+  if (!utf8)
+    return utf8.takeError();
+  return utf8.get();
----------------
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())`.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:827
+
+  const char *script =
+      "from inspect import signature, Parameter, ismethod \n"
----------------
Make this a raw string? Among other benefits, it means clang-format will not reflow it weirdly...


================
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);
----------------
Likewise, should most of these `takeErrors` be `cantFail` too ?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:339
+#if PY_MAJOR_VERSION < 3
+    PyObject *obj = PyObject_CallFunction(m_py_obj, const_cast<char *>(format),
+                                          PythonFormat<T>::get(t)...);
----------------
Maybe it's time for a utility function like `py2_const_cast(format)` ?


================
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);
----------------
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))`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68995