I think it would be better if ShouldLog does all filtering, and Log does unconditional logging. (Hastily) adapting my approach to this PR, we could end up with something like:
<details>
<summary>git diff on 737999e5a8</summary>
diff --git a/src/logging.cpp b/src/logging.cpp
index df6946d661..357a656946 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -605,13 +605,11 @@ bool BCLog::Logger::SetCategoryLogLevel(std::string_view category_str, std::stri
bool util::log::ShouldLog(Category category, Level level)
{
- return LogInstance().WillLogCategoryLevel(static_cast<BCLog::LogFlags>(category), level);
+ BCLog::Logger& logger{LogInstance()};
+ return logger.WillLogCategoryLevel(static_cast<BCLog::LogFlags>(category), level) && logger.Enabled();
}
void util::log::Log(util::log::Entry entry)
{
- BCLog::Logger& logger{LogInstance()};
- if (logger.Enabled()) {
- logger.LogPrintStr(std::move(entry.message), std::move(entry.source_loc), static_cast<BCLog::LogFlags>(entry.category), entry.level, entry.should_ratelimit);
- }
+ LogInstance().LogPrintStr(std::move(entry.message), std::move(entry.source_loc), static_cast<BCLog::LogFlags>(entry.category), entry.level, entry.should_ratelimit);
}
diff --git a/src/util/log.h b/src/util/log.h
index 2e26b3eeec..26f1e7cb94 100644
--- a/src/util/log.h
+++ b/src/util/log.h
@@ -97,9 +97,18 @@ inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags
util::log::Log(util::log::Entry{.category = flag, .level = level, .should_ratelimit = should_ratelimit, .source_loc = std::move(source_loc), .message = std::move(log_msg)});
}
-// Allow __func__ to be used in any context without warnings:
-// NOLINTNEXTLINE(bugprone-lambda-function-name)
-#define LogPrintLevel_(category, level, should_ratelimit, ...) LogPrintFormatInternal(SourceLocation{__func__}, category, level, should_ratelimit, __VA_ARGS__)
+// For Info levels and up, arguments are always evaluated. For Debug and lower,
+// arguments are ONLY evaluated when the log is actually produced.
+#define LogPrintLevel_(category, level, should_ratelimit, ...) \
+ do { \
+ if (util::log::ShouldLog(category, level)) { \
+ /* NOLINTNEXTLINE(bugprone-lambda-function-name) */ \
+ LogPrintFormatInternal(SourceLocation{__func__}, category, level, should_ratelimit, __VA_ARGS__); \
+ } else if ((level) >= util::log::Level::Info) { \
+ /* For Info levels and up, we guarantee that arguments are always evaluated. */ \
+ [](auto&&...) {}(__VA_ARGS__); \
+ } \
+ } while (0)
// Log unconditionally. Uses basic rate limiting to mitigate disk filling attacks.
// Be conservative when using functions that unconditionally log to debug.log!
@@ -109,23 +118,8 @@ inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags
#define LogWarning(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Warning, /*should_ratelimit=*/true, __VA_ARGS__)
#define LogError(...) LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Error, /*should_ratelimit=*/true, __VA_ARGS__)
-// Use a macro instead of a function for conditional logging to prevent
-// evaluating arguments when logging for the category is not enabled.
-
-// Log by prefixing the output with the passed category name and severity level. This logs conditionally if
-// the category is allowed. No rate limiting is applied, because users specifying -debug are assumed to be
-// developers or power users who are aware that -debug may cause excessive disk usage due to logging.
-#define detail_LogIfCategoryAndLevelEnabled(category, level, ...) \
- do { \
- if (util::log::ShouldLog((category), (level))) { \
- bool rate_limit{level >= BCLog::Level::Info}; \
- Assume(!rate_limit); /*Only called with the levels below*/ \
- LogPrintLevel_(category, level, rate_limit, __VA_ARGS__); \
- } \
- } while (0)
-
// Log conditionally, prefixing the output with the passed category name.
-#define LogDebug(category, ...) detail_LogIfCategoryAndLevelEnabled(category, BCLog::Level::Debug, __VA_ARGS__)
-#define LogTrace(category, ...) detail_LogIfCategoryAndLevelEnabled(category, BCLog::Level::Trace, __VA_ARGS__)
+#define LogDebug(category, ...) LogPrintLevel_(category, util::log::Level::Debug, /*should_ratelimit=*/false, __VA_ARGS__)
+#define LogTrace(category, ...) LogPrintLevel_(category, util::log::Level::Trace, /*should_ratelimit=*/false, __VA_ARGS__)
#endif // BITCOIN_UTIL_LOG_H
</details>
This better enables users (e.g. places where we conditionally calculate memusage) to avoid expensive and unnecessary operations, and is imo a more consistent implementation of the interface. I think it also addresses the "This is a minor change in behavior for statements where Logger::WillLogCategoryLevel() returns true and Logger::Enabled() returns false..." paragraphs in the 737999e5a8f3a0c7ebd0385b0f18c109fcd8c49d commit message.
