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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Oct 21 06:34:43 PDT 2019


labath 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();
----------------
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.


================
Comment at: lldb/scripts/Python/python-typemaps.swig:4
 %typemap(in) char ** {
-  using namespace lldb_private;
+
   /* Check if is a list  */
----------------
Could you delete these empty lines too.  You don't have to delete absolutely all of them, if you think they're useful for better visual separation, but I definitely don't think empty lines at the very beginning of a block are useful.


================
Comment at: lldb/scripts/Python/python-wrapper.swig:193-194
 {
-    using namespace lldb_private;
+
 
     if (python_class_name == NULL || python_class_name[0] == '\0' || !session_dictionary_name)
----------------
These even have a double empty line now. :)


================
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()));
 }
----------------
lawrence_danna wrote:
> labath wrote:
> > Do I understand correctly that the idea is to eventually replace these functions with constructor calls too?
> yea
awesome.


================
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");
----------------
lawrence_danna wrote:
> labath wrote:
> > 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.
> Almost everything in PythonDataObjects requires you to call it with the GIL already locked.  The only things that don't are the File methods because they're called by things that don't know anything about python.
> 
> Normally when I need to do something like this I would use dispatch_once, or whatever the LLVM version of that is, so you avoid static initializers, but also initialize the thing in a thread-safe way.
> 
> But in this case we already know that this function can only be called by a thread that has the GIL, so it's safe to just initialize a global variable without further synchronization.
Ok, thanks for explaining. The new comment is much clearer now (though I am not sure if it's even needed TBH).


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1068-1070
+    Twine message =
+        Twine(toCString()) + "\n" +
+        "Traceback unavailble, an error occurred while reading it:\n";
----------------
This won't work. The reason why the Twine class is so efficient, is that it establishes links between various temporary objects. Those temporaries will be destroyed when the enclosing full expression is evaluated. This means it's never safe to store Twine objects as local variables.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1077
+        llvm::handleErrors(backtrace.takeError(), [&](PythonException &E) {
+          backtrace2 = E.ReadBacktraceRecursive(limit - 1);
+        });
----------------
How likely are we to get another exception while trying to retrieve the backtrace? I am wondering if all this complexity is worth it...


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h:26-27
 
+using namespace lldb_private::python;
+
 namespace lldb_private {
----------------
No using declarations in headers.


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:790-793
+  Expected<long long> r = As<long long>(factorial(5ll));
+  bool ok = (bool)r;
+  ASSERT_TRUE(ok);
+  EXPECT_EQ(r.get(), 120);
----------------
EXPECT_THAT_EXPECTED(As<long long>(factorial(5ll)), llvm::HasValue(120))


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:842-843
+  auto g = As<std::string>(globals.GetItem("g"));
+  ASSERT_THAT_EXPECTED(g, llvm::Succeeded());
+  EXPECT_EQ(g.get(), "foobarbaz");
 }
----------------
HasValue(foobarbaz)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69214





More information about the lldb-commits mailing list