[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

No fudnamental issues with this patch (I'm ignoring the CStringArg topic here). I do think that it would be nice to have some more unit tests for the new APIs you're introducing (or elevating to API status). I'm mainly thinking of the PythonScript stuff (a test where we evaluate the script successfully) and the runString functions.



================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:68-70
+    /* If this is true then any exceptions raised by the script will be
+     * cleared with PyErr_Clear().   If false then they will be left for
+     * the caller to clean up */
----------------
I should've mentioned this earlier, but llvm style is to generally use `//` comments <https://llvm.org/docs/CodingStandards.html#comment-formatting>.,


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:261-264
 void PythonBytes::SetBytes(llvm::ArrayRef<uint8_t> bytes) {
   const char *data = reinterpret_cast<const char *>(bytes.data());
-  PyObject *py_bytes = PyBytes_FromStringAndSize(data, bytes.size());
-  PythonObject::Reset(PyRefType::Owned, py_bytes);
+  *this = Take<PythonBytes>(PyBytes_FromStringAndSize(data, bytes.size()));
 }
----------------
Do I understand correctly that the idea is to eventually replace these functions with constructor calls too?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1057
+
+  // global is protected by the GIL
+  static PythonScript read_exception(read_exception_script, "read_exception");
----------------
I just realized I don't actually know what this means. Is it that the object takes the GIL internally? Or that one must have the GIL taken before he starts to interact with it? Maybe it'd be better to clarify this in the class description and just delete these comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1064-1065
+  if (!backtrace) {
+    llvm::consumeError(backtrace.takeError());
+    return "backtrace unavailable";
+  }
----------------
maybe just return `toString(backtrace.takeError())` here?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1512
+Error PythonScript::Init() {
+  if (!function.IsValid()) {
+    PythonDictionary globals(PyInitialValue::Empty);
----------------
Use early return here (`if (valid) return Success;`)


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1516-1517
+    auto builtins = PythonModule::BuiltinsModule();
+    Error error = globals.SetItem("__builtins__", builtins);
+    if (error)
+      return error;
----------------
if (Error error = ...)


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:331
                                           const T &... t) const {
+    using namespace python;
     const char format[] = {'(', PythonFormat<T>::format..., ')', 0};
----------------
Maybe it's time to move the entire PythonObject hierarchy into the python namespace? That would be consistent with some of the newer lldb plugins, which have all of their code inside a sub-namespace of lldb_private..


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:778
+  const char *script;
+  const char *function_name;
+  PythonCallable function;
----------------
I'm wondering if there's a way to avoid passing in the function name argument. Perhaps if the callable is returned as a result of the script ("def foo: ...; return foo")? WDYT?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:789-790
+  llvm::Expected<PythonObject> operator()(Args &&... args) {
+    llvm::Error e = Init();
+    if (e)
+      return std::move(e);
----------------
if (llvm::Error e = ...)


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:1169-1180
+    Status error;
+    llvm::handleAllErrors(
+        return_value.takeError(),
+        [&](PythonException &E) {
+          error.SetErrorString(E.ReadBacktrace());
+          if (!options.GetMaskoutErrors())
+            E.Restore();
----------------
Would something like this also work?
```
llvm::Error error = handleErrors(return_value.takeError(), [](PythonException &E) {
  llvm::Error str = makeStringError(E.ReadBacktrace());
  if (!options.GetMaskoutErrors)
    E.Restore();
  return str;
});
return std::move(error);
```


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:1986-1987
+    reply_pyobj = Take<PythonObject>(setting);
+  else
+    reply_pyobj.Reset();
 
----------------
If doesn't look like this Reset call is really needed here..


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:789-802
+  Expected<PythonObject> r = foo();
+
+  bool failed = !r;
+  ASSERT_TRUE(failed);
+
+  std::string backtrace;
+  llvm::handleAllErrors(r.takeError(), [&](const PythonException &E) {
----------------
Maybe:
```
EXPECT_THAT_EXPECTED(foo(),
  llvm::Failed<PythonException>(testing::Property(&PythonException::ReadBacktrace,
     testing::ContainsRegex(...) )) ));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69214