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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 3 inline comments as done.
mib added inline comments.


================
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 (" +
----------------
JDevlieghere wrote:
> 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.
TBH, I don't see the added value of using `Expected<T>` because I'll still need to check the expected object `!expected` which is the same as doing `!obj`. 

Also, regarding @jingham's suggestion, since `Dispatch`'s return type is templated, it is not guaranteed to have an `operator bool()` method (same with `IsValid()`) that's why the check is done after calling `Dispatch`, on a case by case basis.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:111
+
+    if (sizeof...(Args) > 0) {
+      FormatArgs(format_buffer, args...);
----------------
JDevlieghere wrote:
> FormatArgs has an overload for the no-args case so I don't think you need this? 
I need it since `PyObject_CallMethod` expects a nullptr for the format string when the param pack is empty, not an empty string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107521