[Lldb-commits] [PATCH] D128323: [lldb] Add support for specifying a log handler

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.


================
Comment at: lldb/source/Core/Debugger.cpp:1409-1411
+static std::shared_ptr<LogHandler>
+CreateLogHandler(LogHandlerKind log_handler_kind, int fd, bool should_close,
+                 size_t buffer_size) {
----------------
Many (or most) arguments passed to this function might not be used depending on the kind of LogHandler you're instantiating.  This is fine for now but if we add other handlers in the future that take different argument, it might become very confusing on which one to use.

May be we could use a parameter pack with some template specialization to make this more robust ?


================
Comment at: lldb/source/Core/Debugger.cpp:1444
+        CreateLogHandler(log_handler_kind, GetOutputFile().GetDescriptor(),
+                         !should_close, buffer_size);
   } else {
----------------
I find it confusing to negate `should_close` but to initialize it to `true`.

Since it’s not modified anywhere else, IMO it might be better to inline the value with a comment (`/*should_close=*/`), instead of using the variable.


================
Comment at: lldb/source/Core/Debugger.cpp:1466
+          CreateLogHandler(log_handler_kind, (*file)->GetDescriptor(),
+                           !should_close, buffer_size);
       m_stream_handlers[log_file] = log_handler_sp;
----------------
Ditto


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

https://reviews.llvm.org/D128323