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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:26
 
+static constexpr OptionEnumValueElement g_log_handler_type[] = {
+    {
----------------
Question: how does one see these values and their descriptions? Do the descriptions get displayed anywhere? If they do, then my comments below make sense, if the don't, then ignore below comments and maybe roll all that documentation into the main description for this command.


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:30
+        "stream",
+        "Use the stream log handler",
+    },
----------------
It doesn't seem very clear to the user what "stream" means here. Is says it will use one, but what is a stream? Maybe expand on this will output the log to the debugger stderr or to a file if used with the -f option? Maybe "default" is a better user facing term here?


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:34-35
+        eLogHandlerCircular,
+        "rotating",
+        "Use the rotating log handler",
+    },
----------------
circular makes more sense to use as the name IMHO. Not sure what rotating means for a log handler. Maybe expand on if this is enabled, you can see the cached log lines by using "log dump" and it will keep X amount of messages? Or does the user need to specify a size of a buffer somewhere?


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:40
+        "system",
+        "Use the system log handler",
+    },
----------------
Just saying that we will use the system log handler doesn't really explain what is going on here. Maybe "Log messages to the OS's default system log." 


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:124-126
+          error.SetErrorStringWithFormat(
+              "unrecognized value for log handler '%s'",
+              option_arg.str().c_str());
----------------
Can we also add the valid values that users can choose from in this error message? If the user types "log enable -h wrong", how does the user know what the valid values to use are?


================
Comment at: lldb/source/Commands/Options.td:438-439
+    EnumArg<"Value", "LogHandlerType()">, Desc<"Redirect the log output to a "
+    "specific log handler. The default log handler (stream) writes to the "
+    "debugger output stream or to a file if one is specified with -f.">;
   def log_threadsafe : Option<"threadsafe", "t">, Group<1>,
----------------
Should this be moved to the enum description?


================
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>,
----------------
JDevlieghere wrote:
> 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? 
How can the user see the available options? I always have to type an incorrect value in for enums and then I see a correction saying "you can only specify 'a', 'b', or 'c'"


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

https://reviews.llvm.org/D128323