[Lldb-commits] [PATCH] D155161: [lldb] Convert script native types to StructuredData counterpart

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 2 inline comments as done.
mib added inline comments.


================
Comment at: lldb/bindings/python/python-typemaps.swig:108
+        } else {
+          $1 = $input;
+        }
----------------
bulbazord wrote:
> Do we actually have to do any conversions here? Like to turn it into an SBStructuredData from a python list or dictionary?
No, if the input is a `lldb.SBStructuredData` python object, SWIG takes care of doing the conversion to the C++ type (line 87), but here we just made sure that the input is not an SB type and leave the type conversion to be done in lldb by the Script Interpreter code.


================
Comment at: lldb/include/lldb/API/SBStructuredData.h:34-36
+  static SBStructuredData
+  CreateFromScriptObject(const ScriptedTypedObject obj,
+                         const lldb::SBDebugger &debugger);
----------------
bulbazord wrote:
> Why do we offer both this and `SBDebugger::CreateStructuredDataFromScriptObject`? They do the exact same thing, when would you want to use one over the other?
This actually calls the `SBDebugger` method, but I tried to put myself in the user's shoes and I think I'd go look at the `SBStructuredData` help if I wanted to create a one from a script object. This one is for better discoverability and the other one (`SBDebugger`) is for better ergonomics (since we don't have to pass a debugger).


================
Comment at: lldb/source/API/SBStructuredData.cpp:51-53
+                                         const lldb::SBDebugger &debugger) {
+  return debugger.CreateStructuredDataFromScriptObject(obj);
+}
----------------
bulbazord wrote:
> I already asked why this is necessary, but if it does stick around you'll want to add `LLDB_INSTRUMENT_VA` here too yeah?
This is a `static` method and I don't think we call `LLDB_INSTRUMENT_VA` in that case. Might be wrong though @JDevlieghere ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155161