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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
Comment at: lldb/include/lldb/API/SBBreakpoint.h:18-19
 class ScriptInterpreter;
+namespace python {
+class SWIGBridge;
 }
----------------
mib wrote:
> 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.
I agree, but we can refactor this later. Removing and/or changing forward declarations doesn't break ABI.


================
Comment at: lldb/source/API/SBCommandInterpreter.cpp:37
+namespace lldb_private {
 class CommandPluginInterfaceImplementation : public CommandObjectParsed {
 public:
----------------
mib wrote:
> I find it a bit odd to have this here ... May be we should move this class out the the `API` directory ?
It's a little odd for sure, but it is an implementation detail so having it in the cpp file seems alright. Where would you put it?


================
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);
----------------
mib wrote:
> 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.
Sure, I can add the TODO. I'd rather not add the templates or subclass in this patch as it is complicated enough already.


================
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);
----------------
mib wrote:
> Can we move these in the `python` namespace as well ?
Sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150157