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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
