labath added a comment.
The code itself looks fine to me. Jim, is this ok from a
scripting/compatibility/whatnot perspective?
================
Comment at:
lldb/packages/Python/lldbsuite/test/commands/command/script/TestCommandScript.py:25
+ if sys.version_info[:2] >= (3, 3) or sys.version_info.major < 3:
+ # Test a bunch of different kinds of python callables with
----------------
Do we have to worry about 3.0<=python<=3.3 actually? Unlike 2.7, these have
actually been EOLed already, and I would expect/hope that anyone who is not
stuck at 2.7 has upgraded past them..
================
Comment at: lldb/scripts/Python/python-wrapper.swig:701-702
+ if (!argc) {
+ llvm::consumeError(argc.takeError());
+ return false;
+ }
----------------
There is a possibility to shorten this to `return
errorToBool(argc.takeError())` though I am undecided whether that makes things
clearer or not, so I am mainly writing this to make you aware of that
possibility. I'll leave that up to you to decide whether to use it...
================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:634
+ * accept an arbitrary number */
+ int max_positional_args;
/* the number of positional arguments, including optional ones,
----------------
Make this unsigned, and add a symbolic constant (static constexpr unsigned) for
the "varargs" value.
I've also considered making the vararg-ness more explicit via
`Optional<unsigned>` but (U)INTMAX is actually a pretty good value for the
varargs and it enables you to handle this case with a single comparison, so the
explicitness is probably not worth it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69014/new/
https://reviews.llvm.org/D69014
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
