[Lldb-commits] [PATCH] D128026: [lldb] Add a BufferedLogHandler

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

This is an interesting take that makes more sense than specifying a size in bytes and a "m_circular" since that kind of thing could cut the first message. Saving entire messages as they are logged might make more sense for our circular log messages as we would ensure we have full message lines, though it does come with a "copy each message" cost



================
Comment at: lldb/source/Utility/Log.cpp:361
+    : m_delegate(delegate), m_messages(std::make_unique<std::string[]>(size)),
+      m_size(size) {}
+
----------------
maybe assert size > 0 here to make sure we don't crash in Emit() or set m_size to 1 if size is zero?


================
Comment at: lldb/source/Utility/Log.cpp:366
+void BufferedLogHandler::Emit(llvm::StringRef message) {
+  m_messages[m_next_index++] = message.str();
+  if (m_next_index == m_size)
----------------
If we are going to buffer, I worry about the performance with saving an array or strings that must be copied each time. Though I do like that we are saving messages as entire buffers.

If we go the circular buffer route we can either have one fixed size buffer that we copy into and then try the keep the current insert index, or copy the strings like you are already doing.


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

https://reviews.llvm.org/D128026