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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 11 inline comments as done.
JDevlieghere added inline comments.


================
Comment at: lldb/source/Commands/Options.td:436-437
     Desc<"Set the log to be buffered, using the specified buffer size.">;
+  def log_handler : Option<"handler", "h">, Group<1>,
+    EnumArg<"Value", "LogHandlerType()">, Desc<"Use a custom handler.">;
   def log_threadsafe : Option<"threadsafe", "t">, Group<1>,
----------------
clayborg wrote:
> Maybe "--type" or "--kind" would be better? Handler seems like an internal name. Maybe also mention what it will default to if not specified? Can the user see a list of the valid values or do we need to supply these in the description?
I think the concept of a "log handler" is sufficiently well known that it should be easy for a a (power) user to understand. I'm concerned that "type" and "kind" are a tad too high level, and therefore could easily be confused with the log category and channel. For example, when someone asks to enable the "gdb packet log", is that a category, channel, type or kind? 


================
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) {
----------------
mib wrote:
> 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 ?
What did you have in mind? We don't know a-priori what the type will be. You could template the function in the enum value, but you still wouldn't know which arguments to pass before dispatching to the right function. 


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

https://reviews.llvm.org/D128323