Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13121 )
Change subject: add osmo_use_count API ...................................................................... Patch Set 2: (6 comments) https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h File include/osmocom/core/use_count.h: https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@66 PS2, Line 66: * osmo_use_count_cb_t > Never seen such spacing style before. it's my personal favorite way to space function pointer typedefs. I find the "(*name)" really weird as language design, so I like to set it apart from the name. Especially if a pointer is returned... typedef foo *(*bar)(args); always confuses me... for me it helps to write foo* (* bar )(args); so that bar doesn't look like function arguments or a typecast... Can change if you insist? https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@141 PS2, Line 141: */ > Also looks a bit weird... that's your standard linux torvalds paper scroll way... ok the standard way would be /* * words */ but the known kernel code argument is that this is "braindamaged": /* words * words */ https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@150 PS2, Line 150: count > I'm not arguing agains the requirement / use case. […] ah ok, a naming issue, I understand now. will ponder it... https://gerrit.osmocom.org/#/c/13121/2/tests/use_count/use_count_test.c File tests/use_count/use_count_test.c: https://gerrit.osmocom.org/#/c/13121/2/tests/use_count/use_count_test.c@320 PS2, Line 320: "use_count_test.c" > Why not __FILE__? because it contains the full relative path and depends on where configure was launched from. we could go for 'strrchr(__FILE__, '/') + 1' but that is obscure, unsafe and actually more characters. https://gerrit.osmocom.org/#/c/13121/2/tests/use_count/use_count_test.c@327 PS2, Line 327: log_set_print_filename2(osmo_stderr_target, LOG_FILENAME_NONE); wait a minute, this looks redundant https://gerrit.osmocom.org/#/c/13121/2/tests/use_count/use_count_test.c@326 PS2, Line 326: log_set_print_filename2(osmo_stderr_target, LOG_FILENAME_BASENAME); : log_set_print_filename2(osmo_stderr_target, LOG_FILENAME_NONE); : log_set_print_filename_pos(osmo_stderr_target, LOG_FILENAME_POS_LINE_END); : log_set_print_category(osmo_stderr_target, 1); : log_set_print_category_hex(osmo_stderr_target, 0); : log_set_print_level(osmo_stderr_target, 1); : log_set_use_color(osmo_stderr_target, 0); > Off-topic: I think we need some generic configuration for all unit tests. most of them don't care about logging... -- To view, visit https://gerrit.osmocom.org/13121 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife31e6798b4e728a23913179e346552a7dd338c0 Gerrit-Change-Number: 13121 Gerrit-PatchSet: 2 Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Vadim Yanitskiy <[email protected]> Gerrit-Comment-Date: Mon, 11 Mar 2019 05:55:06 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
