[Lldb-commits] [PATCH] D107521: [lldb/Plugins] Introduce Scripted Interface Factory

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/bindings/python/python-wrapper.swig:291
 
-
     PyErr_Cleaner py_err_cleaner(true);
----------------
Unrelated whitespace change?


================
Comment at: lldb/include/lldb/Interpreter/ScriptedInterface.h:25
+  virtual StructuredData::GenericSP
+  CreatePluginObject(const llvm::StringRef class_name,
+                     ExecutionContext &exe_ctx,
----------------
We generally don't use const when passing by value.


================
Comment at: lldb/include/lldb/Interpreter/ScriptedInterface.h:30
+protected:
+  StructuredData::GenericSP m_object_instance_sp = nullptr;
+};
----------------



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:28
 
+#include "llvm/ADT/Twine.h"
+
----------------
Why is this needed? 


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:38-39
 
-  std::string error_string;
+  Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+                 Locker::FreeLock);
+
----------------
Why was this moved up? The lock should be as tight as possible. 


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:88
+  // TODO: Implement
+  return nullptr;
 }
----------------



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp:99
+              "ScriptedProcess::%s ERROR = %s", __FUNCTION__,
+              message.str().c_str());
+    return StructuredData::DictionarySP();
----------------
Seems like you're always calling `error_with_message` with a constant string, so we can avoid allocating a std::string. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107521