[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/include/lldb/lldb-private-enumerations.h:229
+  eLogHandlerCallback,
+  eLogHandlerRotating,
+#if defined(__APPLE__)
----------------
Maybe "eLogHandlerCircular"?


================
Comment at: lldb/include/lldb/lldb-private-enumerations.h:230
+  eLogHandlerRotating,
+#if defined(__APPLE__)
+  eLogHandlerOSLog,
----------------
remove #if. See inlined comments in https://reviews.llvm.org/D128321


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:37
+    },
+#if defined(__APPLE__)
+    {
----------------
remove #if (see https://reviews.llvm.org/D128321)


================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:41
+        "oslog",
+        "Use the OSLog log handler",
+    },
----------------



================
Comment at: lldb/source/Commands/CommandObjectLog.cpp:167
       buffer_size.Clear();
+      handler = eLogHandlerStream;
       log_options = 0;
----------------
Do we want to define a "eLogHandlerDefault" which points to "eLogHandlerStream"?


================
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>,
----------------
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?


================
Comment at: lldb/source/Core/Debugger.cpp:1418
+    return std::make_shared<RotatingLogHandler>(buffer_size);
+#if defined(__APPLE__)
+  case eLogHandlerOSLog:
----------------
remove #if (see https://reviews.llvm.org/D128321)


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

https://reviews.llvm.org/D128323