Max has posted comments on this change. ( https://gerrit.osmocom.org/12044 )
Change subject: gsm0808: add encoder for cause codes and use it ...................................................................... Patch Set 1: (3 comments) I think the test should be added as well. It's rather easy and the user of this function are outside of the library so it wouldn't be immediately obvious that smth is broken if we don't test for it. https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c File src/gsm/gsm0808_utils.c: https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@51 PS1, Line 51: /*! Encode TS 08.08 AoIP Cause IE I think it's better to reference the same spec as in body of this function. https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@67 PS1, Line 67: buf[1] = (uint8_t) (cause & 0xff); I think it's better to use osmo_store16*() - that way it's immediately obvious which endian is used. Plus it's easier to read. https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@74 PS1, Line 74: return (uint8_t) (msg->tail - old_tail); You know for sure how many bytes are added - just return explicit number from corresponding if clause, there's no need to bother with pointer arithmetic. -- To view, visit https://gerrit.osmocom.org/12044 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: I71d58fad89502a43532f60717ca022c15c73f8bb Gerrit-Change-Number: 12044 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <msur...@sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-Comment-Date: Fri, 30 Nov 2018 15:32:19 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
