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