Modernize Enable/DisableLogChannel interface a bit

[labath]

Summary

Use StringRef and ArrayRef where possible. This adds an accessor to the
Args class to get a view of the arguments as ArrayRef<const char *>.

[zturner]

include/lldb/Interpreter/Args.h

196–197 ↗

(On Diff #89865)

can this be written return makeArrayRef(m_argv).drop_back();?

source/API/SBDebugger.cpp

1129 ↗

(On Diff #89865)

makeArrayRef(categories, len) again seems more readable.

source/Core/Log.cpp

74–80 ↗

(On Diff #89865)

How about

bool exists = llvm::any(entry.second.channel.categories,
        [&](const Log::Category &c) { return c.name.equals_lower(category); }));
if (exists) {
  ...
}

unittests/Interpreter/TestArgs.cpp

174 ↗

(On Diff #89865)

I'd probably use auto here since the function indicates that it's returning an ArrayRef. I don't feel strongly though.

[labath]

Address review comments

[labath]

include/lldb/Interpreter/Args.h

196–197 ↗

(On Diff #89865)

Yes, it looks like it can.

source/Core/Log.cpp

74–80 ↗

(On Diff #89865)

That won't work, because I actually need the relevant iterator to get the associated flag (flags |= cat->flag below).

[labath]

rebase on top of tree

lldb/trunk/include/lldb/Core/Debugger.h

public:
   const char *GetIOHandlerHelpPrologue();
 
   void ClearIOHandlers();
 
   bool GetCloseInputOnEOF() const;
 
   void SetCloseInputOnEOF(bool b);
 
-  bool EnableLog(const char *channel, const char **categories,
-                 const char *log_file, uint32_t log_options,
+  bool EnableLog(llvm::StringRef channel,
+                 llvm::ArrayRef<const char *> categories,
+                 llvm::StringRef log_file, uint32_t log_options,
                  Stream &error_stream);
 
   void SetLoggingCallback(lldb::LogOutputCallback log_callback, void *baton);
 
   //----------------------------------------------------------------------
   // Properties Functions
   //----------------------------------------------------------------------
   enum StopDisassemblyType {

lldb/trunk/include/lldb/Core/Log.h

public:
   // Static accessors for logging channels
   //------------------------------------------------------------------
   static void Register(llvm::StringRef name, Channel &channel);
   static void Unregister(llvm::StringRef name);
 
   static bool
   EnableLogChannel(const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
                    uint32_t log_options, llvm::StringRef channel,
-                   const char **categories, Stream &error_stream);
+                   llvm::ArrayRef<const char *> categories,
+                   Stream &error_stream);
 
   static bool DisableLogChannel(llvm::StringRef channel,
-                                const char **categories, Stream &error_stream);
+                                llvm::ArrayRef<const char *> categories,
+                                Stream &error_stream);
 
   static bool ListChannelCategories(llvm::StringRef channel, Stream &stream);
 
   static void DisableAllLogChannels(Stream *feedback_strm);
 
   static void ListAllLogChannels(Stream *strm);
 
   //------------------------------------------------------------------

lldb/trunk/include/lldb/Interpreter/Args.h

public:
   ///
   /// @return
   ///     An array of NULL terminate C string argument pointers that
   ///     also has a terminating NULL C string pointer
   //------------------------------------------------------------------
   const char **GetConstArgumentVector() const;
 
   //------------------------------------------------------------------
+  /// Gets the argument as an ArrayRef. Note that the return value does *not*
+  /// have a nullptr const char * at the end, as the size of the list is
+  /// embedded in the ArrayRef object.
+  //------------------------------------------------------------------
+  llvm::ArrayRef<const char *> GetArgumentArrayRef() const {
+    return llvm::makeArrayRef(m_argv).drop_back();
+  }
+
+  //------------------------------------------------------------------
   /// Appends a new argument to the end of the list argument list.
   ///
   /// @param[in] arg_cstr
   ///     The new argument as a NULL terminated C string.
   ///
   /// @param[in] quote_char
   ///     If the argument was originally quoted, put in the quote char here.
   //------------------------------------------------------------------

lldb/trunk/source/API/SBDebugger.cpp

 SBTypeSynthetic SBDebugger::GetSyntheticForType(SBTypeNameSpecifier type_name) {
   if (!type_name.IsValid())
     return SBTypeSynthetic();
   return SBTypeSynthetic(
       DataVisualization::GetSyntheticForType(type_name.GetSP()));
 }
 #endif // LLDB_DISABLE_PYTHON
 
+static llvm::ArrayRef<const char *> GetCategoryArray(const char **categories) {
+  if (categories == nullptr)
+    return {};
+  size_t len = 0;
+  while (categories[len] != nullptr)
+    ++len;
+  return llvm::makeArrayRef(categories, len);
+}
+
 bool SBDebugger::EnableLog(const char *channel, const char **categories) {
   if (m_opaque_sp) {
     uint32_t log_options =
         LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
     StreamString errors;
-    return m_opaque_sp->EnableLog(channel, categories, nullptr, log_options,
-                                  errors);
+    return m_opaque_sp->EnableLog(channel, GetCategoryArray(categories), "",
+                                  log_options, errors);
   } else
     return false;
 }
 
 void SBDebugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
                                     void *baton) {
   if (m_opaque_sp) {
     return m_opaque_sp->SetLoggingCallback(log_callback, baton);
   }
 }

lldb/trunk/source/Commands/CommandObjectLog.cpp

bool DoExecute(Args &args, CommandReturnObject &result) override {
     const std::string channel = args[0].ref;
     args.Shift(); // Shift off the channel
     char log_file[PATH_MAX];
     if (m_options.log_file)
       m_options.log_file.GetPath(log_file, sizeof(log_file));
     else
       log_file[0] = '\0';
     bool success = m_interpreter.GetDebugger().EnableLog(
-        channel.c_str(), args.GetConstArgumentVector(), log_file,
-        m_options.log_options, result.GetErrorStream());
+        channel, args.GetArgumentArrayRef(), log_file, m_options.log_options,
+        result.GetErrorStream());
     if (success)
       result.SetStatus(eReturnStatusSuccessFinishNoResult);
     else
       result.SetStatus(eReturnStatusFailed);
     return result.Succeeded();
   }
 
   CommandOptions m_options;
bool DoExecute(Args &args, CommandReturnObject &result) override {
     }
 
     const std::string channel = args[0].ref;
     args.Shift(); // Shift off the channel
     if (channel == "all") {
       Log::DisableAllLogChannels(&result.GetErrorStream());
       result.SetStatus(eReturnStatusSuccessFinishNoResult);
     } else {
-      if (Log::DisableLogChannel(channel, args.GetConstArgumentVector(),
+      if (Log::DisableLogChannel(channel, args.GetArgumentArrayRef(),
                                  result.GetErrorStream()))
         result.SetStatus(eReturnStatusSuccessFinishNoResult);
     }
     return result.Succeeded();
   }
 };
 
 class CommandObjectLogList : public CommandObjectParsed {

lldb/trunk/source/Core/Debugger.cpp

 void Debugger::SetLoggingCallback(lldb::LogOutputCallback log_callback,
                                   void *baton) {
   // For simplicity's sake, I am not going to deal with how to close down any
   // open logging streams, I just redirect everything from here on out to the
   // callback.
   m_log_callback_stream_sp.reset(new StreamCallback(log_callback, baton));
 }
 
-bool Debugger::EnableLog(const char *channel, const char **categories,
-                         const char *log_file, uint32_t log_options,
+bool Debugger::EnableLog(llvm::StringRef channel,
+                         llvm::ArrayRef<const char *> categories,
+                         llvm::StringRef log_file, uint32_t log_options,
                          Stream &error_stream) {
   const bool should_close = true;
   const bool unbuffered = true;
 
   std::shared_ptr<llvm::raw_ostream> log_stream_sp;
   if (m_log_callback_stream_sp) {
     log_stream_sp = m_log_callback_stream_sp;
     // For now when using the callback mode you always get thread & timestamp.
     log_options |=
         LLDB_LOG_OPTION_PREPEND_TIMESTAMP | LLDB_LOG_OPTION_PREPEND_THREAD_NAME;
-  } else if (log_file == nullptr || *log_file == '\0') {
+  } else if (log_file.empty()) {
     log_stream_sp = std::make_shared<llvm::raw_fd_ostream>(
         GetOutputFile()->GetFile().GetDescriptor(), !should_close, unbuffered);
   } else {
     auto pos = m_log_streams.find(log_file);
     if (pos != m_log_streams.end())
       log_stream_sp = pos->second.lock();
     if (!log_stream_sp) {
       llvm::sys::fs::OpenFlags flags = llvm::sys::fs::F_Text;

lldb/trunk/source/Core/Log.cpp

static void ListCategories(Stream &stream, const ChannelMap::value_type &entry) {
   stream.Format("Logging categories for '{0}':\n", entry.first());
   stream.Format("  all - all available logging categories\n");
   stream.Format("  default - default set of logging categories\n");
   for (const auto &category : entry.second.channel.categories)
     stream.Format("  {0} - {1}\n", category.name, category.description);
 }
 
 static uint32_t GetFlags(Stream &stream, const ChannelMap::value_type &entry,
-                  const char **categories) {
+                         llvm::ArrayRef<const char *> categories) {
   bool list_categories = false;
   uint32_t flags = 0;
-  for (size_t i = 0; categories[i]; ++i) {
-    if (llvm::StringRef("all").equals_lower(categories[i])) {
+  for (const char *category : categories) {
+    if (llvm::StringRef("all").equals_lower(category)) {
       flags |= UINT32_MAX;
       continue;
     }
-    if (llvm::StringRef("default").equals_lower(categories[i])) {
+    if (llvm::StringRef("default").equals_lower(category)) {
       flags |= entry.second.channel.default_flags;
       continue;
     }
-    auto cat = llvm::find_if(entry.second.channel.categories,
-                             [&](const Log::Category &c) {
-                               return c.name.equals_lower(categories[i]);
-                             });
+    auto cat = llvm::find_if(
+        entry.second.channel.categories,
+        [&](const Log::Category &c) { return c.name.equals_lower(category); });
     if (cat != entry.second.channel.categories.end()) {
       flags |= cat->flag;
       continue;
     }
-    stream.Format("error: unrecognized log category '{0}'\n", categories[i]);
+    stream.Format("error: unrecognized log category '{0}'\n", category);
     list_categories = true;
   }
   if (list_categories)
     ListCategories(stream, entry);
   return flags;
 }
 
 void Log::Channel::Enable(Log &log,
void Log::Unregister(llvm::StringRef name) {
   auto iter = g_channel_map->find(name);
   assert(iter != g_channel_map->end());
   iter->second.channel.Disable(UINT32_MAX);
   g_channel_map->erase(iter);
 }
 
 bool Log::EnableLogChannel(
     const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
-    uint32_t log_options, llvm::StringRef channel, const char **categories,
-    Stream &error_stream) {
+    uint32_t log_options, llvm::StringRef channel,
+    llvm::ArrayRef<const char *> categories, Stream &error_stream) {
   auto iter = g_channel_map->find(channel);
   if (iter == g_channel_map->end()) {
     error_stream.Format("Invalid log channel '{0}'.\n", channel);
     return false;
   }
-  uint32_t flags = categories && categories[0]
-                       ? GetFlags(error_stream, *iter, categories)
-                       : iter->second.channel.default_flags;
+  uint32_t flags = categories.empty()
+                       ? iter->second.channel.default_flags
+                       : GetFlags(error_stream, *iter, categories);
   iter->second.channel.Enable(iter->second.log, log_stream_sp, log_options,
                               flags);
   return true;
 }
 
-bool Log::DisableLogChannel(llvm::StringRef channel, const char **categories,
+bool Log::DisableLogChannel(llvm::StringRef channel,
+                            llvm::ArrayRef<const char *> categories,
                             Stream &error_stream) {
   auto iter = g_channel_map->find(channel);
   if (iter == g_channel_map->end()) {
     error_stream.Format("Invalid log channel '{0}'.\n", channel);
     return false;
   }
-  uint32_t flags = categories && categories[0]
-                       ? GetFlags(error_stream, *iter, categories)
-                       : UINT32_MAX;
+  uint32_t flags = categories.empty()
+                       ? UINT32_MAX
+                       : GetFlags(error_stream, *iter, categories);
   iter->second.channel.Disable(flags);
   return true;
 }
 
 bool Log::ListChannelCategories(llvm::StringRef channel, Stream &stream) {
   auto ch = g_channel_map->find(channel);
   if (ch == g_channel_map->end()) {
     stream.Format("Invalid log channel '{0}'.\n", channel);

lldb/trunk/tools/lldb-server/LLDBServerUtilities.cpp

bool LLDBServerUtilities::SetupLogging(const std::string &log_file,
   log_channels.split(channel_array, ":", /*MaxSplit*/ -1, /*KeepEmpty*/ false);
   for (auto channel_with_categories : channel_array) {
     StreamString error_stream;
     Args channel_then_categories(channel_with_categories);
     std::string channel(channel_then_categories.GetArgumentAtIndex(0));
     channel_then_categories.Shift(); // Shift off the channel
 
     bool success = Log::EnableLogChannel(
-        log_stream_sp, log_options, channel.c_str(),
-        channel_then_categories.GetConstArgumentVector(), error_stream);
+        log_stream_sp, log_options, channel,
+        channel_then_categories.GetArgumentArrayRef(), error_stream);
     if (!success) {
       fprintf(stderr, "Unable to open log file '%s' for channel \"%s\"\n",
               log_file.c_str(), channel_with_categories.str().c_str());
       return false;
     }
   }
   return true;
 }

lldb/trunk/unittests/Core/LogTest.cpp

TEST(LogTest, Register) {
   Log::Register("chan", test_channel);
   Log::Unregister("chan");
 }
 
 TEST(LogTest, Unregister) {
   llvm::llvm_shutdown_obj obj;
   Log::Register("chan", test_channel);
   EXPECT_EQ(nullptr, test_channel.GetLogIfAny(FOO));
-  const char *cat1[] = {"foo", nullptr};
+  const char *cat1[] = {"foo"};
   std::string message;
   std::shared_ptr<llvm::raw_string_ostream> stream_sp(
       new llvm::raw_string_ostream(message));
   StreamString err;
   EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat1, err));
   EXPECT_NE(nullptr, test_channel.GetLogIfAny(FOO));
   Log::Unregister("chan");
   EXPECT_EQ(nullptr, test_channel.GetLogIfAny(FOO));
 }
 
 TEST_F(LogChannelTest, Enable) {
   EXPECT_EQ(nullptr, test_channel.GetLogIfAll(FOO));
   std::string message;
   std::shared_ptr<llvm::raw_string_ostream> stream_sp(
       new llvm::raw_string_ostream(message));
   StreamString err;
-  EXPECT_FALSE(Log::EnableLogChannel(stream_sp, 0, "chanchan", nullptr, err));
+  EXPECT_FALSE(Log::EnableLogChannel(stream_sp, 0, "chanchan", {}, err));
   EXPECT_EQ("Invalid log channel 'chanchan'.\n", err.GetString());
   err.Clear();
 
-  EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", nullptr, err));
-  EXPECT_EQ("", err.GetString());
+  EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
+  EXPECT_EQ("", err.GetString()) << "err: " << err.GetString().str();
   EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
   EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
 
-  const char *cat2[] = {"bar", nullptr};
+  const char *cat2[] = {"bar"};
   EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat2, err));
   EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO | BAR));
 
-  const char *cat3[] = {"baz", nullptr};
+  const char *cat3[] = {"baz"};
   EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat3, err));
   EXPECT_TRUE(err.GetString().contains("unrecognized log category 'baz'"))
       << "err: " << err.GetString().str();
   EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO | BAR));
 }
 
 TEST_F(LogChannelTest, EnableOptions) {
   EXPECT_EQ(nullptr, test_channel.GetLogIfAll(FOO));
   std::string message;
   std::shared_ptr<llvm::raw_string_ostream> stream_sp(
       new llvm::raw_string_ostream(message));
   StreamString err;
   EXPECT_TRUE(Log::EnableLogChannel(stream_sp, LLDB_LOG_OPTION_VERBOSE, "chan",
-                                    nullptr, err));
+                                    {}, err));
 
   Log *log = test_channel.GetLogIfAll(FOO);
   ASSERT_NE(nullptr, log);
   EXPECT_TRUE(log->GetVerbose());
 }
 
 TEST_F(LogChannelTest, Disable) {
   EXPECT_EQ(nullptr, test_channel.GetLogIfAll(FOO));
-  const char *cat12[] = {"foo", "bar", nullptr};
+  const char *cat12[] = {"foo", "bar"};
   std::string message;
   std::shared_ptr<llvm::raw_string_ostream> stream_sp(
       new llvm::raw_string_ostream(message));
   StreamString err;
   EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", cat12, err));
   EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO | BAR));
 
-  const char *cat2[] = {"bar", nullptr};
+  const char *cat2[] = {"bar"};
   EXPECT_TRUE(Log::DisableLogChannel("chan", cat2, err));
   EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
   EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
 
-  const char *cat3[] = {"baz", nullptr};
+  const char *cat3[] = {"baz"};
   EXPECT_TRUE(Log::DisableLogChannel("chan", cat3, err));
   EXPECT_TRUE(err.GetString().contains("unrecognized log category 'baz'"))
       << "err: " << err.GetString().str();
   EXPECT_NE(nullptr, test_channel.GetLogIfAll(FOO));
   EXPECT_EQ(nullptr, test_channel.GetLogIfAll(BAR));
   err.Clear();
 
-  EXPECT_TRUE(Log::DisableLogChannel("chan", nullptr, err));
+  EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
   EXPECT_EQ(nullptr, test_channel.GetLogIfAny(FOO | BAR));
 }
 
 TEST_F(LogChannelTest, List) {
   StreamString str;
   EXPECT_TRUE(Log::ListChannelCategories("chan", str));
   std::string expected =
       R"(Logging categories for 'chan':

 
 TEST_F(LogChannelTest, LogThread) {
   // Test that we are able to concurrently write to a log channel and disable
   // it.
   std::string message;
   std::shared_ptr<llvm::raw_string_ostream> stream_sp(
       new llvm::raw_string_ostream(message));
   StreamString err;
-  EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", nullptr, err));
+  EXPECT_TRUE(Log::EnableLogChannel(stream_sp, 0, "chan", {}, err));
 
   Log *log = test_channel.GetLogIfAll(FOO);
 
   // Start logging on one thread. Concurrently, try disabling the log channel.
   std::thread log_thread([log] { LLDB_LOG(log, "Hello World"); });
-  EXPECT_TRUE(Log::DisableLogChannel("chan", nullptr, err));
+  EXPECT_TRUE(Log::DisableLogChannel("chan", {}, err));
   log_thread.join();
 
   // The log thread either managed to write to the log in time, or it didn't. In
   // either case, we should not trip any undefined behavior (run the test under
   // TSAN to verify this).
   EXPECT_TRUE(stream_sp->str() == "" || stream_sp->str() == "Hello World\n")
       << "str(): " << stream_sp->str();
 }

lldb/trunk/unittests/Interpreter/TestArgs.cpp

TEST(ArgsTest, AppendArguments) {
   EXPECT_STREQ("4", args.GetArgumentVector()[3]);
   EXPECT_EQ(nullptr, args.GetArgumentVector()[4]);
   EXPECT_STREQ("1", args.GetArgumentAtIndex(0));
   EXPECT_STREQ("2", args.GetArgumentAtIndex(1));
   EXPECT_STREQ("3", args.GetArgumentAtIndex(2));
   EXPECT_STREQ("4", args.GetArgumentAtIndex(3));
 }
 
+TEST(ArgsTest, GetArgumentArrayRef) {
+  Args args("foo bar");
+  auto ref = args.GetArgumentArrayRef();
+  ASSERT_EQ(2u, ref.size());
+  EXPECT_STREQ("foo", ref[0]);
+  EXPECT_STREQ("bar", ref[1]);
+}
+
 TEST(ArgsTest, StringToBoolean) {
   bool success = false;
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, nullptr));
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("on"), false, nullptr));
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("yes"), false, nullptr));
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("1"), false, nullptr));
 
   EXPECT_TRUE(Args::StringToBoolean(llvm::StringRef("true"), false, &success));