[Lldb-commits] [PATCH] D159314: [lldb] Introduce OperatingSystem{, Python}Interface and make us it

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

This change seems mostly red in that we're removing a lot of things. What are the replacements? I also see you're removing the API lock acquisition in a few places, where does that now occur?



================
Comment at: lldb/bindings/python/python-wrapper.swig:266-279
+  switch (arg_info.get().max_positional_args) {
+    case 1:
+      // FIXME: Since this is used by different scripting affordances, they can have different number
+      // of argument but also different types of arguments (i.e SBExecutionContect vs SBProcess)
+      // We need to have a more reliable way to forward positional arguments.
+      result = pfunc(SWIGBridge::ToSWIGWrapper(exe_ctx_sp->GetProcessSP()));
+      break;
----------------
Is there no way to check the types of the positional args? This seems like it might be a source of future subtle bugs or unexpected behavior.


================
Comment at: lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp:112-114
+    //    return llvm::createStringError(
+    //        llvm::inconvertibleErrorCode(),
+    //        "Failed to create scripted thread interface.");
----------------
nit: Remove commented out code.

side note, maybe OperatingSystemPython should have a static factory method so we can actually return a meaningful error like this? Since the constructor can fail, we might end up with a half-initialized object. Doesn't have to be in this change, I think it would make sense for a follow-up.


================
Comment at: lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp:123-128
+    //    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+    //                                   "Failed to create script object.");
+    return;
+  if (!owned_script_object_sp->IsValid())
+    //    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+    //                                   "Created script object is invalid.");
----------------
Same here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159314