Add logging callback #201
Add logging callback #201
Conversation
KGronek-Pubnub
requested review from
Xavrax,
parfeon and
seba-aln
as code owners
core/pubnub_log.h
Outdated
| #define PUBNUB_LOG_PRINTF(LVL, ...) \ | ||
| do { \ | ||
| if (pubnub_log_callback) { \ | ||
| char logMessage[1024]; \ |
There was a problem hiding this comment.
@KGronek-Pubnub Buffer size won't be an issue here?
Also, I'm concerned about change in signature because of this.
There was a problem hiding this comment.
@parfeon yeah I was afraid that this change in signature could cause some potential issues :/ thanks
I will change this logic to happen in PUBNUB_LOG macro instead.
As for the buffer size, I can change to calculate it dynamically.
core/pubnub_log.h
Outdated
| #define PUBNUB_LOG(LVL, ...) \ | ||
| do { \ | ||
| if (LVL <= PUBNUB_LOG_LEVEL) { \ | ||
| if (pubnub_log_callback) { \ |
There was a problem hiding this comment.
Are we sure that we want to have this dynamic decision making?
Shouldn't we make it a compilation flag?
I mean it makes sense when a flag would be set to TRUE (to check if somebody added the callback) but it makes a difference as it is called very frequently for our customers that doesn't need this feature.
Probably the performance loss won't be even noticed but I want to start the discussion about it.
There was a problem hiding this comment.
@KGronek-Pubnub @parfeon
What are your opinions?
There was a problem hiding this comment.
Sure I could rework it to be a flag instead, just I think there are already so many compilation flags, so for end users it might be very difficult to understand and compile the version they need.
I don't think validation check can make any performance loss, so it's more a design decision here.
There was a problem hiding this comment.
C-core is mostly configurable via the compilation flags. It's not the best design but still I would stick to that concept.
But I won't block this PR if there are no other voices.
|
Anyway overall LGTM - just discuss and resolve the conversation. |
# Conflicts: # CMakeLists.txt
feat(log): add possibility to replace default log function with a callback provided by the user