[Lldb-commits] [PATCH] D117490: [lldb] Make logging machinery type-safe

[Pavel Labath via Phabricator via lldb-commits]

labath marked 4 inline comments as done.
labath added a comment.

This should be ready for a full review now.



================
Comment at: lldb/include/lldb/Utility/Logging.h:15
-// Log Bits specific to logging in lldb
-#define LIBLLDB_LOG_PROCESS (1u << 1)
-#define LIBLLDB_LOG_THREAD (1u << 2)
----------------
Bit zero was unused, so we still have one bit left, but I've created a new typedef for holding the category flags, so extending it should be easy.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemoteLog.cpp:17
 
 static constexpr Log::Category g_categories[] = {
+    {{"async"}, {"log asynchronous activity"}, uint32_t(GDBR_LOG_ASYNC)},
----------------
labath wrote:
> clayborg wrote:
> > Should the Log::Category be templatized with GDBRLog and then we just store GDBRLog enum values instead of uint32_t values?
> The problem with that is that it would require templatizing pretty much all of the logging code (as it deals with categories in one way or another), but I agree that we should at least make the category definitions nice. It should be possible to keep templates here, but then convert to integers when we get to the core logging code, but i did not want to get entagled in that for the prototype.
I think I've mostly solved this by templatizing the `Log::Category` constructor (instead of the whole class). That way the generic code remains template free, and there are no casts in the user code. The constructor converts the enum values to integers, after verifying that they are of the right type.


================
Comment at: lldb/source/Utility/Logging.cpp:28-40
+    {{"dyld"},
+     {"log shared library related activities"},
+     LLDBLog::DynamicLoader},
+    {{"event"},
+     {"log broadcaster, listener and event queue activities"},
+     LLDBLog::Events},
+    {{"expr"}, {"log expressions"}, LLDBLog::Expressions},
----------------
This would also be a good time to bikeshed on the names, as some of these are fairly inconsistent (`dyld` vs `DynamicLoader`, `event` vs `Events`, `jit` vs `JITLoader`). I think we could standardise on the shorter, user-facing ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117490