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