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

MmkvLogger creates log strings even if logging is disabled #710

Open
isaacl opened this issue Jul 31, 2024 · 4 comments · May be fixed by #711
Open

MmkvLogger creates log strings even if logging is disabled #710

isaacl opened this issue Jul 31, 2024 · 4 comments · May be fixed by #711

Comments

@isaacl
Copy link

isaacl commented Jul 31, 2024

The string_format method seems like an anti-pattern... Is is possible to move all formatting to platform log implementations, which skip the string format overhead when not in use?

Copy link

Guten Tag, Hans here.

Note

New features, bugfixes, updates and other improvements are all handled mostly by @mrousavy in his free time.
To support @mrousavy, please consider 💖 sponsoring him on GitHub 💖.
Sponsored issues will be prioritized.

@mrousavy
Copy link
Owner

Sure it is possible - I'd gladly accept PRs that change/fix this.

@isaacl
Copy link
Author

isaacl commented Aug 2, 2024

It's an unneeded heap allocation plus the formatting logic added to every command... IMO, a wrapper around a performance-driven library shouldn't be doing that. You don't want to fix it?

Your string_format() method is complicated and I assume there's a reason you hand wrote that instead of passing all args to android/ios loggers .. but I don't understand why its there. So it'd be hard for me to write a PR.

Perhaps you could use preprocessor directives in your cpp Log function...

@isaacl isaacl linked a pull request Aug 2, 2024 that will close this issue
@isaacl
Copy link
Author

isaacl commented Aug 2, 2024

I'm no c guru -- but I put my ideas into a PR. Feel free to use some or none of them.

It appears NSLog needs to be guarded by preprocessor guard in Release builds if you don't want to log.

Android Log probably skips INFO logs on release though I haven't tested.

Finally, I also showed putting the preprocessor directive in the shared cpp. I don't know if that works though.

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 a pull request may close this issue.

2 participants