[Lldb-commits] [PATCH] D13617: Fix ref-counting of Python objects

[Zachary Turner via lldb-commits]

zturner added a comment.

Let me know what you think about my responses.  Anything I didn't specifically call out I'll fix in the next version of the patch.


================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:73-79
@@ -73,2 +72,9 @@
 
-    class PythonObject
+enum class PyRefType
+{
+    Borrowed, // We are not given ownership of the incoming PyObject.
+              // We cannot safely hold it without calling Py_INCREF.
+    Owned     // We have ownership of the incoming PyObject.  We should
+              // not call Py_INCREF.
+};
+
----------------
clayborg wrote:
> clayborg wrote:
> > Might be nicer to move into the PyRef class and just name these "Type" and "InitialValue"?
> It seems like we don't need an enum for this, this can just be a bool named "dont_incref" or some other name?
In theory.  I used an enum because if you read the Python documentation, they explicitly use the terminology of Borrowed and New references.  I don't want people to have to think about how to translate the Python documentation into a boolean value.  This way the code speaks the same language as the documentation, and it's hard to make a mistake.

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:73-85
@@ -73,2 +72,15 @@
 
-    class PythonObject
+enum class PyRefType
+{
+    Borrowed, // We are not given ownership of the incoming PyObject.
+              // We cannot safely hold it without calling Py_INCREF.
+    Owned     // We have ownership of the incoming PyObject.  We should
+              // not call Py_INCREF.
+};
+
+enum class PyInitialValue
+{
+    Invalid,
+    Empty
+};
+
----------------
zturner wrote:
> clayborg wrote:
> > clayborg wrote:
> > > Might be nicer to move into the PyRef class and just name these "Type" and "InitialValue"?
> > It seems like we don't need an enum for this, this can just be a bool named "dont_incref" or some other name?
> In theory.  I used an enum because if you read the Python documentation, they explicitly use the terminology of Borrowed and New references.  I don't want people to have to think about how to translate the Python documentation into a boolean value.  This way the code speaks the same language as the documentation, and it's hard to make a mistake.
Possible, but it only really applies for Dictionary and List, or at least the way we use it it does.

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:87
@@ -75,1 +86,3 @@
+
+class PyRef
     {
----------------
clayborg wrote:
> Why did we need to rename PythonObject to PyRef? It can be done, but seems different from the the rest of the classes here. Everything else is spelled out, I would prefer this stay as PythonObject.
Less to type for one thing, and I thought it made it clearer that it's a very thin wrapper that can be trivially copied without any overhead.

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:192
@@ +191,3 @@
+
+        operator PyObject *() const { return m_py_obj; }
+
----------------
clayborg wrote:
> I really don't like these implicit conversions as they make for bad things that can happen. We already see issues with Reset as you mention above. I would rather just have a "PyObject *get() const;" method that we use to explicitly do this. It gets event worse when you already have an implicit bool operator...
Fair enough, I can fix that.  Do you want me to remove the implicit bool conversion operator too?  The `bool` conversion operator is weird, because it only checks for null, and not `Py_None`, so it's different than calling `IsNULLOrNone`.  In practice I think it would be fine to call `IsNULLOrNone`, and at one point while this CL is in progress I had changed the client code to do that, but it ended up being really ugly so I put the bool conversion back.

In any case, should I just leave the bool operator and remove the `PyObject` operator?


http://reviews.llvm.org/D13617