Skip to content

Conversation

swebb2066
Copy link
Contributor

No description provided.

Comment on lines +541 to +543
if (!LogLog::isDebugEnabled())
;
else if (appender != 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit cleaner:

Suggested change
if (!LogLog::isDebugEnabled())
;
else if (appender != 0)
if (LogLog::isDebugEnabled() && appender != nullptr)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty if avoids extra indentation and guards the else case which is also a LogLog::debug statement

@rm5248
Copy link
Contributor

rm5248 commented May 15, 2024

Since we have a lot of internal log messages that only log strings, would it make sense to make a vararg or parameter packed version of LogLog::debug()? That way you could do something like LogLog::debug("Opening file ", filename, " failed"); which would automatically concatenate the strings. That would also make the LOGLOG_DEBUG macro more useful.

@swebb2066
Copy link
Contributor Author

would it make sense to make a vararg or parameter packed version of LogLog::debug()?

I do not know how to use parameter packs and achieve avoiding message building when debug is disabled. I believe internal debug logging is currently sparse (limited to infrequently executed code) to reduce overhead. The LogLog::isDebugEnabled() is much higher overhead than Logger::isDebugEnabledFor(logger)

@swebb2066 swebb2066 merged commit 5354f9c into master May 17, 2024
@swebb2066 swebb2066 deleted the condition_internal_logging branch May 17, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants