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.

Sign up for GitHub

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

Jump to bottom

Ignore log messages for disabled log levels #3006

Merged
merged 1 commit into from
Aug 11, 2020
Merged

Ignore log messages for disabled log levels #3006

merged 1 commit into from
Aug 11, 2020

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Aug 8, 2020

This fix is required when merging #2911 with log level Info for tests into master.

@uklotzde uklotzde requested a review from Holzhaus August 8, 2020 16:06
@Holzhaus
Copy link
Member

Holzhaus commented Aug 8, 2020

Can you elaborate a bit what this does? It looks like for each log level the writeFlags are set in the code above. What messages does this affect?

@uklotzde
Copy link
Contributor Author

uklotzde commented Aug 8, 2020

Without this early return a debug assertion is triggered in writeToLog() for Debug messages if the log level is set to Info during tests:

DEBUG_ASSERT(flags & (WriteFlag::StdErr | WriteFlag::File));

@uklotzde uklotzde mentioned this pull request Aug 11, 2020
Merged
7 tasks
Holzhaus
Holzhaus approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for CI.

@uklotzde
Copy link
Contributor Author

uklotzde commented Aug 11, 2020 •
edited
Loading

No issues so far even without this PR. Close and reopen when actually needed? Still needed!

@Holzhaus
Copy link
Member

Holzhaus commented Aug 11, 2020 •
edited
Loading

Shouldn't this also give a tiny performance improvement? I think we can merge this anyway but feel free to close this if you disagree.

@uklotzde
Copy link
Contributor Author

Still needed. I experience debug assertions when running Mixxx with log level Info instead of Debug.

@Holzhaus
Copy link
Member

Ok, CI failure is unrelated, so let's merge this. Thanks!

@Holzhaus Holzhaus merged commit 1a50e3a into mixxxdj:master Aug 11, 2020
@uklotzde uklotzde deleted the log_level_filtering branch August 14, 2020 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Holzhaus approved these changes

Assignees
No one assigned
Labels
minor bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@uklotzde @Holzhaus