<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p><a href="https://gerrit.osmocom.org/12044">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c">File src/gsm/gsm0808_utils.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@51">Patch Set #1, Line 51:</a> <code style="font-family:monospace,monospace">/*! Encode TS 08.08 AoIP Cause IE</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it's better to reference the same spec as in body of this function.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@67">Patch Set #1, Line 67:</a> <code style="font-family:monospace,monospace">               buf[1] = (uint8_t) (cause & 0xff);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/12044/1/src/gsm/gsm0808_utils.c@74">Patch Set #1, Line 74:</a> <code style="font-family:monospace,monospace">     return (uint8_t) (msg->tail - old_tail);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12044">change 12044</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/12044"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I71d58fad89502a43532f60717ca022c15c73f8bb </div>
<div style="display:none"> Gerrit-Change-Number: 12044 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 30 Nov 2018 15:32:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>