[Lldb-commits] [PATCH] D95711: [lldb/Interpreter] Add ScriptInterpreter Wrapper for ScriptedProcess

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/include/lldb/API/SBData.h:129
 
+  lldb::DataExtractorSP GetOpaque() const;
+
----------------
You cannot expose non-SB classes in public methods because that would make them part of the API. 


================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:531
 
+#pragma mark ScriptedProcessInterface
+
----------------
Please use Doxygen groups for this so everyone can benefit from it and not just a single IDE. 

```
/// ScriptedProcessInterface
/// @{
...
/// @}
```


================
Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:534
+  virtual StructuredData::GenericSP
+  ScriptedProcess_CreatePluginObject(const char *class_name,
+                                     lldb::TargetSP target_sp,
----------------
It doesn't seem like these methods belong in ScriptInterpreter. Can they go into their own class of which the ScriptInterpreter holds a single instance? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95711