react-native-mmkv icon indicating copy to clipboard operation
react-native-mmkv copied to clipboard

MmkvLogger creates log strings even if logging is disabled

Open isaacl opened this issue 1 year ago • 4 comments

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?

isaacl avatar Jul 31 '24 21:07 isaacl

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.

maintenance-hans[bot] avatar Jul 31 '24 21:07 maintenance-hans[bot]

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

mrousavy avatar Jul 31 '24 21:07 mrousavy

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 avatar Aug 02 '24 01:08 isaacl

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.

isaacl avatar Aug 02 '24 01:08 isaacl