-
Notifications
You must be signed in to change notification settings - Fork 30
[c++] Move logging functionality to a separate object library #4377
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
base: main
Are you sure you want to change the base?
Conversation
4a4c6bd to
bfa7213
Compare
d704788 to
bdf5305
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4377 +/- ##
==========================================
- Coverage 86.36% 86.36% -0.01%
==========================================
Files 139 139
Lines 21093 21093
Branches 15 15
==========================================
- Hits 18218 18216 -2
- Misses 2875 2877 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jp-dark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small optional comments, but otherwise looks good.
| inline constexpr std::string_view LOG_PATTERN{"%^[%Y-%m-%d %H:%M:%S.%e] [%n] [Process: %P] [Thread: %t] [%l] %v%$"}; | ||
| inline constexpr std::string_view CONSOLE_LOGGER{"tiledbsoma"}; | ||
| inline constexpr std::string_view FILE_LOGGER{"tiledbsoma-file"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the value in switching these from const std::string to inline constexpr std::string_view? The previous method seems more direct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on the string length using a std::string you may have a heap allocation. In this case this is not relevant because LOG_PATTERN is passed by value to spdlog so any optimization is ineffective. i will revert to the old method.
Issue and/or context: SOMA-820
Changes:
This PR includes:
Notes for Reviewer:
This PR is the first from a series focused on separating non SOMA specific functionality to a separate object library.