zturner added inline comments.
================
Comment at: include/lldb/Core/Log.h:74
+ llvm::ArrayRef<Category> categories,
+ uint32_t default_flags, std::atomic<Log *> &log_ptr);
+ static void Unregister(llvm::StringRef name);
----------------
Not sure I like the idea of this being a `std::atomic`. Why is this necessary?
================
Comment at: source/Commands/CommandObjectLog.cpp:278
+ bool success = true;
+ for (auto &entry : args.entries())
+ success = success && Log::ListChannelCategories(
----------------
`const auto&`?
================
Comment at: source/Core/Log.cpp:57
+
+void ListCategories(Stream &stream, const ChannelMap::value_type &channel) {
+ stream.Format("Logging categories for '{0}':\n", channel.first());
----------------
LLVM guidelines say only classes, structures, and types can be in an anonymous
namespace, but not functions. So this should go out of the anonymous
namespace, but be declared as global statics.
Same thing withsubsequent functions.
================
Comment at: source/Core/Log.cpp:72
+ if (llvm::StringRef("all").equals_lower(categories[i])) {
+ flags |= ~0;
+ continue;
----------------
This is a little clearer if you just say `flags = 0xFFFFFFFF` IMO.
================
Comment at: source/Core/Log.cpp:266
const std::shared_ptr<llvm::raw_ostream> &log_stream_sp,
- uint32_t log_options, const char *channel, const char **categories,
+ uint32_t log_options, llvm::StringRef channel, const char **categories,
Stream &error_stream) {
----------------
How hard would it be to change `categories` to `ArrayRef<StringRef>`, or
alternatively `ArrayRef<const char*>`?
================
Comment at: source/Core/Log.cpp:284-285
+ if (ch->second.log.m_mask_bits.Get()) {
+ ch->second.log.SetStream(log_stream_sp);
+ ch->second.log_ptr.store(&ch->second.log, std::memory_order_release);
+ }
----------------
Here's an example of why I think we need a mutex instead of `std::atomic`.
There seems to be a race condition here if two threads call `EnableLogChannel`
and `DisableLogChannel` at the same time. You can get into a situation where
the stream is null but the log is enabled.
================
Comment at: source/Plugins/SymbolFile/DWARF/LogChannelDWARF.cpp:32
void LogChannelDWARF::Initialize() {
- PluginManager::RegisterPlugin(GetPluginNameStatic(),
- GetPluginDescriptionStatic(),
- LogChannelDWARF::CreateInstance);
-}
-
-void LogChannelDWARF::Terminate() {
- PluginManager::UnregisterPlugin(LogChannelDWARF::CreateInstance);
-}
-
-LogChannel *LogChannelDWARF::CreateInstance() { return new LogChannelDWARF(); }
-
-lldb_private::ConstString LogChannelDWARF::GetPluginNameStatic() {
- return SymbolFileDWARF::GetPluginNameStatic();
+ Log::Register("dwarf", g_categories, DWARF_LOG_DEFAULT, g_log_channel);
}
----------------
Where is `g_log_channel_` allocated? AFAICT it's always `nullptr`.
https://reviews.llvm.org/D29895
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
