At the moment logging in LLDB done in the following way:
Log* log = GetLogIfAllCategoriesSet(…);
This approach is clean and easy to understand but have the disadvantage of being a bit verbose. What is the general opinion about changing it to something like this?
Logger log = GetLogIfAllCategoriesSet(…);
The idea would be to return a new type of object from GetLogIfAllCategoriesSet with small size (size of a pointer) what will check if the log category is enabled. From efficiency perspective this change would have no effect and it will simplify the writing of the logging statements.
Logger would just contain a pointer to a Log object and forward all call to that object if that one isn’t null. Additionally it will have a method to check for nullness of the underlying log object if we want to do some calculation only if the logging is enabled.
P.S.: Other possible simplification in the logging system would be to use LogIfAllCategoriesSet but it require the specification of the log channel at each call and have a very minor overhead because of checking for the enabled log categories at each call.
From an efficiency perspective, the arguments to Printf will still need to be evaluated. Some of those arguments touch multiple areas and will require significant effort to change into a new format, which is essentially the exact same as we have now.
Was there not a decision to stick with what we have now when this came up a few weeks ago? Clean and easy to understand over verbose any day of the week in my view.
I don’t remember to any discussion about it but I might just missed it (don’t see it in the archive either).
From the efficiency perspective in most of the case evaluating the arguments for Printf should be very fast (printing local variable) and the few case where it isn’t the case we can keep the condition (using “if (log.Enabled()) log.Printf(…)”). From readability perspective I think ignoring the “if (log)” in most of the case won’t hurt and it will eliminate the possibility of missing a check what will cause a crash.
We could solve booth the efficiency concerns and the conciseness with a macro. (Gasp!)
After the previous discussion I agree that evaluating the arguments is unacceptable. But you are correct here that a macro would solve this. In fact, most C++ log libraries use macros I guess for this very reason.
I decided to make some macros for the windows plugin which you can look at it in ProcessWindowsLog.h.
There are some issues that are not obvious how to solve though. For example, the macros I wrote in ProcessWindowsLog cannot be used outside of my plugin. This is because each plugin statically defines its own channels as well as defines its own global Log object. If this were to be done in a way that there were one set of macros that all current and future generic code and plugins could use, I think it would require a fairly substantial refactor.
Thank you for the link to the previous discussion and the description of the Windows logging. I like the idea of the macro based logging on Windows but agree that the explicit log channel definition is a bit too verbose.
Currently I would prefer a mixed solution with ‘Log* log = …; LOG_IF(log, “pattern”, …);’ for the usual case and ‘Log* log = …; LOG_IF_ANY(log, categories, “pattern”, …);’ if we want to log to different log channels. I believe it will improve the readability quite a lot, but would only make sense if we can apply it to the full code base within a reasonably short period of time to avoid confusion with the multiple log patterns.
There are also a few competing log implementations. There are ones like in "source/Core/Logging.cpp" which are hard coded, and then there is a plugin version as you can see in LogChannelDWARF.cpp. The latter is the newer way to implement custom log channels without hand coding a copy of "source/Core/Logging.cpp" for your uses. Code that uses "source/Core/Logging.cpp" was never ported to the new plugin version of logging because it was just busy work that would organize the code a bit better, but do nothing functionally to improve anything.
So if we do make changes we would move over to using the plugin version from "source/Core/Log.cpp" and add any macros or useful things to "include/lldb/Core/Log.h".