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

[Greg Clayton via lldb-commits]

clayborg added a comment.

I would like to see PythonObject used in place of PyRef, feel free to remove the operator bool and PyObject, and then the rest of the mostly indent and style cleanups.


================
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.
+};
+
----------------
zturner wrote:
> 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.
Ok, no problem then.

================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:192
@@ +191,3 @@
+
+        operator PyObject *() const { return m_py_obj; }
+
----------------
zturner wrote:
> 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?
Yeah, feel free to pull the operator bool as well. It can do bad things as well. IsValid() is what we use elsewhere, thought we may want more python specific things. We might want IsNULL(), IsNone() or IsNULLOrNone() depending on who calls it. Feel free to clean this up.


http://reviews.llvm.org/D13617