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

get rid of broken CStaticLoggerBase #19363

Merged
merged 1 commit into from Mar 15, 2021

Conversation

Montellese
Copy link
Member

Description

When introducing spdlog I wanted to implement it in some classes for showcasing (e.g. CWebServer and settings related classes). Because some of these classes were instantiated hundreds or even thousands of times I wanted to use a static logger. To simplify the integration and reduce code duplication I wrote a CStaticLoggerBase class which took a logger name and provided a static s_logger member variable.

Obviously this approach doesn't work because all classes deriving from CStaticLoggerBase share the same static members so the logger stored in s_logger would only be instantiated once for the first instantiated class deriving from CStaticLoggerBase and all other classes deriving from it would use the same logger with a mismatching name.

This should probably also be considered for backporting because it results in bad log messages which might be confusing when trying to read the logs to track down an issue.

The approach I chose now is to provide a static GetLogger() helper in every class which previously derived from CStaticLoggerBase.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@Montellese Montellese added this to the N* 20.0 Alpha 1 milestone Mar 8, 2021
@Montellese Montellese added this to In progress in General (roadmap) via automation Mar 8, 2021
General (roadmap) automation moved this from In progress to Reviewer approved Mar 8, 2021
Copy link
Member

@garbear garbear left a comment

Choose a reason for hiding this comment

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

The diff looks good, I had one question above.

@Montellese Montellese merged commit 0c095e0 into xbmc:master Mar 15, 2021
General (roadmap) automation moved this from Reviewer approved to Done Mar 15, 2021
@Montellese Montellese deleted the bugfix/cstaticloggerbase branch March 15, 2021 13:01
@Montellese
Copy link
Member Author

@DaveTBlake should I backport this for Matrix to fix the logger names in kodi.log?

@DaveTBlake
Copy link
Member

Yes please do, we will be working with log files from Matrix for some time to come so may as well have any benifits.

@Montellese
Copy link
Member Author

Yes please do, we will be working with log files from Matrix for some time to come so may as well have any benifits.

Please see #19428.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants