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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits