Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not fmt::format log strings unless required #145

Merged
merged 1 commit into from
Sep 22, 2022
Merged

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Sep 20, 2022

We have a two-stage log formatting scheme where we pass the user's formatting spec into fmt::format and then use our own format string "{}{}" to prepend the Celerity log context. We currently call fmt::format unconditionally, even if spdlog will drop the message due to the current global log level.

This PR makes the calls to fmt and spdlog conditional on the global log level.

@fknorr fknorr requested a review from psalz September 20, 2022 13:25
@fknorr fknorr self-assigned this Sep 20, 2022
@fknorr fknorr requested a review from BlackMark29A September 21, 2022 09:38
Copy link

@BlackMark29A BlackMark29A left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

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

Good change, thanks!

@fknorr fknorr merged commit f0b53ce into master Sep 22, 2022
@fknorr fknorr deleted the log-conditional branch September 22, 2022 10:55
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