[Lldb-commits] [PATCH] D139252: [lldb/Plugins] Introduce Scripted Platform Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/Platform/CMakeLists.txt:9
 add_subdirectory(POSIX)
+add_subdirectory(scripted)
 add_subdirectory(QemuUser)
----------------



================
Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.cpp:208
+                                ProcessInstanceInfoList &proc_infos) {
+  CheckInterpreterAndScriptObject();
+  StructuredData::DictionarySP dict_sp = GetInterface().ListProcesses();
----------------
What's your intention by calling this function? In a release build this is still going to crash. Is there a way to initialize the platform with an invalid `m_script_object_sp` or `m_interpreter`? If so then we should have proper error handling. If not then these can be regular asserts.


================
Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.h:49-52
+                         lldb_private::Target *target, // Can be nullptr, if
+                         // nullptr create a new
+                         // target, else use
+                         // existing one
----------------
This should be a Doxygen comment. 


================
Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.h:70-74
+  void CheckInterpreterAndScriptObject() const;
+  ScriptedPlatformInterface &GetInterface() const;
+  llvm::Expected<ProcessInstanceInfo>
+  ParseProcessInfo(StructuredData::Dictionary &dict, lldb::pid_t pid) const;
+  static bool IsScriptLanguageSupported(lldb::ScriptLanguage language);
----------------
This is hard to parse, do they really need to be grouped together? I'd add a newline between them like the public functions. 


================
Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.h:76
+
+  // Member variables.
+  const ScriptedMetadata *m_scripted_metadata = nullptr;
----------------
Useless comment


================
Comment at: lldb/source/Plugins/Platform/scripted/ScriptedPlatform.h:80
+  lldb_private::StructuredData::ObjectSP m_script_object_sp = nullptr;
+  //@}
+};
----------------
Missing group opener (`@{`), but you can just remove it if you do the same for line 76


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

https://reviews.llvm.org/D139252