clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
See inlined comments.
================
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:
> 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?
================
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
+};
+
----------------
Might be nicer to move into the PyRef class and just name these "Type" and
"InitialValue"?
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:87
@@ -75,1 +86,3 @@
+
+class PyRef
{
----------------
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.
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:90-91
@@ -76,4 +89,4 @@
public:
- PythonObject () :
- m_py_obj(NULL)
+ PyRef()
+ : m_py_obj(nullptr)
{
----------------
indent is wrong here
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:192
@@ +191,3 @@
+
+ operator PyObject *() const { return m_py_obj; }
+
----------------
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...
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:206-211
@@ -162,8 +205,8 @@
public:
- PythonString ();
- PythonString (PyObject *o);
- PythonString (const PythonObject &object);
- PythonString (llvm::StringRef string);
- PythonString (const char *string);
- virtual ~PythonString ();
+ PythonString();
+ PythonString(PyRefType type, PyObject *o);
+ PythonString(const PythonString &object);
+ explicit PythonString(llvm::StringRef string);
+ explicit PythonString(const char *string);
+ ~PythonString() override;
----------------
indent is wrong here
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:234-238
@@ -188,8 +233,7 @@
public:
-
- PythonInteger ();
- PythonInteger (PyObject* py_obj);
- PythonInteger (const PythonObject &object);
- PythonInteger (int64_t value);
- virtual ~PythonInteger ();
+ PythonInteger();
+ PythonInteger(PyRefType type, PyObject *o);
+ PythonInteger(const PythonInteger &object);
+ explicit PythonInteger(int64_t value);
+ ~PythonInteger() override;
----------------
indent is wrong here
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:258-261
@@ -211,6 +257,6 @@
public:
- PythonList();
- PythonList (PyObject* py_obj);
- PythonList (const PythonObject &object);
- virtual ~PythonList ();
+ PythonList(PyInitialValue value);
+ PythonList(PyRefType type, PyObject *o);
+ PythonList(const PythonList &list);
+ ~PythonList() override;
----------------
indent is wrong here
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:284-287
@@ -237,6 +283,6 @@
public:
- PythonDictionary();
- PythonDictionary (PyObject* object);
- PythonDictionary (const PythonObject &object);
- virtual ~PythonDictionary ();
+ PythonDictionary(PyInitialValue value);
+ PythonDictionary(PyRefType type, PyObject *o);
+ PythonDictionary(const PythonDictionary &dict);
+ ~PythonDictionary() override;
----------------
indent is wrong here
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:304
@@ -261,15 +303,3 @@
- typedef bool (*DictionaryIteratorCallback)(PythonString* key,
PythonDictionary* dict);
-
- PythonList
- GetKeys () const;
-
- PythonString
- GetKeyAtPosition (uint32_t pos) const;
-
- PythonObject
- GetValueAtPosition (uint32_t pos) const;
-
- void
- SetItemForKey (const PythonString &key, PyObject *value);
+ void SetItemForKey(const PythonString &key, PyObject *value);
----------------
We should use "const PyRef &object" here instead of "PyObject *value". I know
the old code used PyObject, but it is probably better to switch over.
================
Comment at: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:306
@@ -275,4 +305,3 @@
- void
- SetItemForKey (const PythonString &key, const PythonObject& value);
+ void SetItemForKey(const char *key, PyObject *value);
----------------
We should use "const PyRef &object" here instead of "PyObject *value". I know
the old code used PyObject, but it is probably better to switch over.
================
Comment at:
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp:183-201
@@ -204,1 +182,21 @@
+ScriptInterpreterPython::ScriptInterpreterPython(CommandInterpreter
&interpreter)
+ : ScriptInterpreter(interpreter, eScriptLanguagePython)
+ , IOHandlerDelegateMultiline("DONE")
+ , m_saved_stdin()
+ , m_saved_stdout()
+ , m_saved_stderr()
+ , m_main_module()
+ , m_lldb_module()
+ , m_session_dict(PyInitialValue::Invalid)
+ , m_sys_module_dict(PyInitialValue::Invalid)
+ , m_run_one_line_function()
+ , m_run_one_line_str_global()
+ ,
m_dictionary_name(interpreter.GetDebugger().GetInstanceName().AsCString())
+ , m_terminal_state()
+ , m_active_io_handler(eIOHandlerNone)
+ , m_session_is_active(false)
+ , m_pty_slave_is_open(false)
+ , m_valid_session(true)
+ , m_lock_count(0)
+ , m_command_thread_state(nullptr)
{
----------------
please move : to previous line and commas to the end.
http://reviews.llvm.org/D13617
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
