[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:93
+
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+
----------------
You can move this into the `if` on line 98, or even just inline it. 


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:100-103
+    LLDB_LOGF(
+        log,
+        "ScriptedProcess::%s failed to listen for m_async_broadcaster events",
+        __FUNCTION__);
----------------



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:138
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+
+  LLDB_LOGF(log, "ScriptedProcess::%s ()", __FUNCTION__);
----------------
I see this newline between `log` and `LLDB_LOGF` across this patch and while I don't mind (it's consistent) I'm curious why it's here. The next line is immediately using `log` and it other places in this patch where a variable is used directly after there is (usually) no newline.


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:205
+      bool is_running = false;
+      do {
+        switch (event_type) {
----------------
Do we need this loop at all? It looks like nothing is ever setting `is_running` to true, so effectively this runs just the once? The two nested loops make it pretty hard to follow what the control flow is here. 


================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:236
+        }
+      } while (is_running); // do while
+    } else {
----------------



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:316-345
+  bool resume = false;
+  Status status;
+
+  // FIXME: Fetch data from thread.
+  const StateType thread_resume_state = eStateRunning;
+
+  LLDB_LOGF(log, "ScriptedProcess::%s thread_resume_state = %s", __FUNCTION__,
----------------



================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.h:128
+  lldb_private::StructuredData::ObjectSP m_script_object_sp = nullptr;
+  Broadcaster m_async_broadcaster;
+  lldb::ListenerSP m_async_listener_sp;
----------------
Can we add a Doxygen comment (group) that explains what these things are and what they're used for? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384