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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
