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

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere marked 6 inline comments as done.
JDevlieghere added a subscriber: jingham.
JDevlieghere added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:26
 
+static constexpr OptionEnumValueElement g_log_handler_type[] = {
+    {
----------------
clayborg wrote:
> 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.
The values are printed in the help output:

```
       -h <log-handler> ( --log-handler <log-handler> )
            Specify a log handler which determines where log messages are written.
            Values: default | circular | os
```

The usage/description is not. As far as I can tell, it isn't printed anywhere. I talked it over with @jingham and it should get printed in the `help <log-hander>` output but currently that's not the case. 

```
(lldb) help <log-handler>
  <log-handler> -- The log handle that will be used to write out log messages.
```

To make that work we need to tie the actual enum to the ArgumentTableEntry in in CommandObject.cpp. I think that's something we should do, but that's outside the scope of this patch. 


================
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:
> 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'"
This is part of the help output. 


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

https://reviews.llvm.org/D128323