Attention is currently required from: Hoernchen.
laforge has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/30633 )
Change subject: logging: add log level cache
......................................................................
Patch Set 5:
(4 comments)
Patchset:
PS5:
thanks, it looks reasonably simple in general, which is good.
I am not entirely happy about the frequent #if !EMBEDDED sprinkled around, and I think we
can do better by, for example #if-ing out the body of the function log_update_cache(). So
the caller sites are unaffected, but the function will turn into a nop as it has an empty
body. Same could be done to log_cache_update_all().
Also, I think it would be better if the functions and symbols related to the cache could
share a common prefix like log_cache_* - where now we have log_update_cache vs.
log_cache_update_all.
All not super critical, but I'd hope Eric and/or others agree the code would look
cleaner that way.
File src/logging.c:
https://gerrit.osmocom.org/c/libosmocore/+/30633/comment/09cb298b_c956f3c7
PS5, Line 124: /
please add doxygen-style comment desribing function and arguments
https://gerrit.osmocom.org/c/libosmocore/+/30633/comment/38b5ad17_7c45d4d6
PS5, Line 1037: log_update_cache
this is not guarded by !EMBEDDED, unlike other invocations?
https://gerrit.osmocom.org/c/libosmocore/+/30633/comment/e03ebd25_54aca467
PS5, Line 1534: log
you just checked it is != NULL above, so that should be a NOP?
--
To view, visit
https://gerrit.osmocom.org/c/libosmocore/+/30633
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I35f8dd9127dd6e7feae392094fd6b3ce2d32558d
Gerrit-Change-Number: 30633
Gerrit-PatchSet: 5
Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 16 Dec 2022 17:01:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment