[Lldb-commits] [PATCH] D95713: [lldb/Plugins] Add ScriptedProcess Process Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:117
+  if (m_interpreter)
+    m_pid = m_interpreter->GetScriptedProcessInterface().GetProcessID();
+}
----------------
`m_interpreter->GetScriptedProcessInterface()` seems to repeated a lot. Maybe it's worth having a private helper method that wraps it?

```
ScriptedProcess::GetInferface() { 
  return m_interpreter->GetScriptedProcessInterface();
}
```


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:153
+  if (!m_interpreter) {
+    size = LLDB_INVALID_ADDRESS;
+    error.SetErrorString("No interpreter.");
----------------
Remembering to set the size seems error prone. I would use a `ScopeExit` (and clear it at the end on success). 


================
Comment at: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:47
+
+    @skipUnlessDarwin
+    def test_launch_scripted_process(self):
----------------
I think this should work on all platforms? 


================
Comment at: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:54
+        self.assertTrue(target, VALID_TARGET)
+        scripted_process_example_relpath = "../../../../examples/python/scripted_process/my_scripted_process.py"
+        os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
----------------
can you make this more portable with `os.path.join` or `os.dirname`? 


================
Comment at: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:55
+        scripted_process_example_relpath = "../../../../examples/python/scripted_process/my_scripted_process.py"
+        os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+        self.runCmd("command script import " + os.path.join(self.getSourceDir(),
----------------
Why is this needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95713