[Lldb-commits] [PATCH] D59976: [Python] Remove Python include from ScriptInterpreterPython.h

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h:327
 
-  class Locker : public ScriptInterpreterLocker {
-  public:
----------------
You could still keep this class inside ScriptInterpreterPython, and just forward-declare it in the header.
The cpp file can define it as `class ScriptInterpreterPython::Locker`.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h:394-410
+  std::unique_ptr<PythonFile> m_saved_stdin;
+  std::unique_ptr<PythonFile> m_saved_stdout;
+  std::unique_ptr<PythonFile> m_saved_stderr;
+  std::unique_ptr<PythonObject> m_main_module;
+  std::unique_ptr<PythonObject> m_lldb_module;
+  std::unique_ptr<PythonDictionary> m_session_dict;
+  std::unique_ptr<PythonDictionary> m_sys_module_dict;
----------------
Instead of pimpl-ing each individual field, what if we just have one struct which contains all of these fields, and pimpl that?

This should make things more efficient (less heap traffic), and allow you to cut down on the number of forward declarations (particularly the ones from python headers worry me).


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/lldb-python.h:35
 
+// Include locale before Python so _PY_PORT_CTYPE_UTF8_ISSUE doesn't cause
+// macro redefinitions.
----------------
Python headers are fun. :)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D59976