[Lldb-commits] [PATCH] D95712: [lldb/bindings] Add Python ScriptedProcess base class to lldb module

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/examples/python/scripted_process/scripted_process.py:18
+
+    Attributes:
+        args (lldb.SBStructuredData): Dictionary holding arbitrary values
----------------
Doesn't the python documentation generator infer the attributes and methods from this file? If not, is it worth having to keep it in sync?


================
Comment at: lldb/examples/python/scripted_process/scripted_process.py:67-71
+        self.process_id = 0
+        self.memory_regions = []
+        self.threads = []
+        self.loaded_images = []
+        self.stops = []
----------------
Personally I don't think these belongs in the base class:

 1. The corresponding methods don't return these lists. 
 2. More importantly, if they're attributes then that becomes part of the interface. Now every implementation needs to populate e.g. the `threads` list while they might want to compute that lazily, like some of the OS plugin implementations do. 


================
Comment at: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:27
+    def test_python_plugin_package(self):
+        """Test that the lldb python module has a `plugins.scripted_process`
+        package."""
----------------
None of the expect calls seems to be checking that the package exists and can be imported? 


================
Comment at: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py:33
+        self.expect('script dir(lldb.plugins)',
+                    patterns=["scripted_process"])
+
----------------
It's not clear to me what this is actually testing. The same is true for the other `expect` calls below. Maybe a comment or add a failure string to the expect calls. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95712