[Lldb-commits] [PATCH] D150157: [lldb] Mark most SBAPI methods involving private types as protected or private

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM with some comments.



================
Comment at: lldb/include/lldb/API/SBBreakpoint.h:18-19
 class ScriptInterpreter;
+namespace python {
+class SWIGBridge;
 }
----------------
We've talked about this offline, but I think we should stay language agnostic in the SBAPI, so exposing a python namespace here is bothering me a little bit.


================
Comment at: lldb/source/API/SBCommandInterpreter.cpp:37
+namespace lldb_private {
 class CommandPluginInterfaceImplementation : public CommandObjectParsed {
 public:
----------------
I find it a bit odd to have this here ... May be we should move this class out the the `API` directory ?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:64-90
+class SWIGBridge {
+public:
+  static PythonObject ToSWIGWrapper(std::unique_ptr<lldb::SBValue> value_sb);
+  static PythonObject ToSWIGWrapper(lldb::ValueObjectSP value_sp);
+  static PythonObject ToSWIGWrapper(lldb::TargetSP target_sp);
+  static PythonObject ToSWIGWrapper(lldb::ProcessSP process_sp);
+  static PythonObject ToSWIGWrapper(lldb::ThreadPlanSP thread_plan_sp);
----------------
Although this is nested in the `python` namespace, I think `SWIGBridge` should be a templated class defined in the `ScriptedInterpreter` header and this class should be instead renamed as `SWIGPythonBridge` and be a specialization of the `SWIGBridge` class. We can do that as a follow-up but a `TODO` comment would be nice.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:248-254
+void *LLDBSWIGPython_CastPyObjectToSBData(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBBreakpoint(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBAttachInfo(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBLaunchInfo(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBError(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBValue(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBMemoryRegionInfo(PyObject *data);
----------------
Can we move these in the `python` namespace as well ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150157