[Lldb-commits] [PATCH] D107521: [lldb/Plugins] Introduce Scripted Interface Factory

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

Nobody will be surprised that I like the templated solution for the dispatch method :-) A small comment similar to Jim's but otherwise this looks good to me.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:38-39
 
-  std::string error_string;
+  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+                 Locker::FreeLock);
+
----------------
JDevlieghere wrote:
> Why was this moved up? The lock should be as tight as possible. 
This comment is marked as done, but the lock is still taken at the beginning of the function. 


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:80
 
-  return static_cast<bool>(*should_stop);
-}
-
-Status ScriptedProcessPythonInterface::Stop() {
-  return GetStatusFromMethod("stop");
-}
-
-Status ScriptedProcessPythonInterface::GetStatusFromMethod(
-    llvm::StringRef method_name) {
-  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
-                 Locker::FreeLock);
-
-  if (!m_object_instance_sp)
-    return Status("Python object ill-formed.");
-
-  if (!m_object_instance_sp)
-    return Status("Cannot convert Python object to StructuredData::Generic.");
-  PythonObject implementor(PyRefType::Borrowed,
-                           (PyObject *)m_object_instance_sp->GetValue());
-
-  if (!implementor.IsAllocated())
-    return Status("Python implementor not allocated.");
-
-  PythonObject pmeth(
-      PyRefType::Owned,
-      PyObject_GetAttrString(implementor.get(), method_name.str().c_str()));
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  if (!pmeth.IsAllocated())
-    return Status("Python method not allocated.");
-
-  if (PyCallable_Check(pmeth.get()) == 0) {
-    if (PyErr_Occurred())
-      PyErr_Clear();
-    return Status("Python method not callable.");
-  }
-
-  if (PyErr_Occurred())
-    PyErr_Clear();
-
-  PythonObject py_return(PyRefType::Owned,
-                         PyObject_CallMethod(implementor.get(),
-                                             method_name.str().c_str(),
-                                             nullptr));
-
-  if (PyErr_Occurred()) {
-    PyErr_Print();
-    PyErr_Clear();
-    return Status("Python method could not be called.");
-  }
-
-  if (PyObject *py_ret_ptr = py_return.get()) {
-    lldb::SBError *sb_error =
-        (lldb::SBError *)LLDBSWIGPython_CastPyObjectToSBError(py_ret_ptr);
-
-    if (!sb_error)
-      return Status("Couldn't cast lldb::SBError to lldb::Status.");
-
-    Status status = m_interpreter.GetStatusFromSBError(*sb_error);
-
-    if (status.Fail())
-      return Status("error: %s", status.AsCString());
-
-    return status;
+  if (!obj || !obj->IsValid() || error.Fail()) {
+    return error_with_message(llvm::Twine("Null or invalid object (" +
----------------
jingham wrote:
> It seems like we also do this bit every time we do Dispatch.  Maybe Dispatch could check the outgoing obj & obj->IsValid and if they aren't then it can put an appropriate message in the "error" object.  Then you would just have to check error.Fail().
> 
> That would also simplify the logging.   You could always dump the error's GetCString to your error_with_message.  Right now, you don't log the error's GetCString in the case where the object is valid, but error.Fail is true, which might end up dropping some useful info.
I had a similar observation but I was thinking that Dispatch should return an `Expected<StructuredData::ObjectSP>`. That way you can be more specific in your error message too and differentiate between the object being null, invalid or another error.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:39-45
+    lldb::SBError *sb_error = reinterpret_cast<lldb::SBError *>(
+        LLDBSWIGPython_CastPyObjectToSBError(p.get()));
+
+    if (!sb_error)
+      error.SetErrorString("Couldn't cast lldb::SBError to lldb::Status.");
+    else
+      error = m_interpreter.GetStatusFromSBError(*sb_error);
----------------
This is a common pattern in llvm, but totally up to preference 


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:111
+
+    if (sizeof...(Args) > 0) {
+      FormatArgs(format_buffer, args...);
----------------
FormatArgs has an overload for the no-args case so I don't think you need this? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107521