Patch Set 1: Code-Review-1 (2 comments)
https://gerrit.osmocom.org/#/c/6316/1/include/osmocom/core/fsm.h File include/osmocom/core/fsm.h: Line 90: char *id; hm. We cannot change a "const char *" pointer? I always thought it states that the destination buffer is 'const' and not that the pointer itself is 'const'. But well, if it cannot be solved in a different way, we'll have to go ahead with this :/ https://gerrit.osmocom.org/#/c/6316/1/src/fsm.c File src/fsm.c: Line 205: bool osmo_fsm_inst_update_id(struct osmo_fsm_inst *fi, const char *id) in general we return 0 on success and negative on error. Only predicate-type functions like "is_this_a_valid_name()" deviate from this. Your function breaks that general assumption, and it is difficult as a caller to know if one should check for negative returns or false, which is an easy way to introduce subtle code bugs. Please return 0 on success and -EINVAL or the like on error. -- To view, visit https://gerrit.osmocom.org/6316 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic216e5b11d4440f8e106a297714f4f06c1152945 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: daniel <dwillm...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
