[Lldb-commits] [PATCH] D69014: [LLDB] bugfix: command script add -f doesn't work for some callables

[Pavel Labath via Phabricator via lldb-commits]

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