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

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added inline comments.


================
Comment at: lldb/scripts/Python/python-extensions.swig:683-686
                 if (desc_len > 0)
-                    return lldb_private::PythonString(llvm::StringRef(desc, desc_len)).release();
+                    return PythonString(llvm::StringRef(desc, desc_len)).release();
                 else
+                    return PythonString("").release();
----------------
labath wrote:
> You don't have to do this if you don't want, but I couldn't help noticing that the "else" branch is completely unnecessary in all of these scriptlets.
good point.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1077
+        llvm::handleErrors(backtrace.takeError(), [&](PythonException &E) {
+          backtrace2 = E.ReadBacktraceRecursive(limit - 1);
+        });
----------------
labath wrote:
> How likely are we to get another exception while trying to retrieve the backtrace? I am wondering if all this complexity is worth it...
It's actually not that unusual that someone writes a `__str__` method for their exception which is broken and also throws an exception.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1077
+        llvm::handleErrors(backtrace.takeError(), [&](PythonException &E) {
+          backtrace2 = E.ReadBacktraceRecursive(limit - 1);
+        });
----------------
lawrence_danna wrote:
> labath wrote:
> > How likely are we to get another exception while trying to retrieve the backtrace? I am wondering if all this complexity is worth it...
> It's actually not that unusual that someone writes a `__str__` method for their exception which is broken and also throws an exception.
actually, it looks like `traceback.print_exception` already takes care of that situation, so you're right it's not very useful.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:26-27
 
+using namespace lldb_private::python;
+
 namespace lldb_private {
----------------
labath wrote:
> No using declarations in headers.
even an "Impl" header?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69214