<p style="white-space: pre-wrap; word-wrap: break-word;">feedback on my feedback to your feedback is welcome...</p><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/include/osmocom/bsc/gsm_data.h">File include/osmocom/bsc/gsm_data.h:</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/osmo-bsc/+/25972/1/include/osmocom/bsc/gsm_data.h@1439">Patch Set #1, Line 1439:</a> <code style="font-family:monospace,monospace">      CHAN_COUNTS2_N</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">is this simply to flag the end? please document so, since it's a bit confusing as this is an enum en […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">it's to know how large to define the array (N as in number of indexes)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/include/osmocom/bsc/gsm_data.h@1442">Patch Set #1, Line 1442:</a> <code style="font-family:monospace,monospace">extern const struct value_string chan_count2_strs[];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Lots of new infra, sounds like all this can be moved to a new header file.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh i thought i had kept those *str* functions in a separate patch, just needed it for debugging and haven't decided yet whether to commit at all. these shouldn't be here</p><p style="white-space: pre-wrap; word-wrap: break-word;">but ack, maybe a separate .h/.c pair could be nice</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/abis_rsl.c">File src/osmo-bsc/abis_rsl.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/osmo-bsc/+/25972/1/src/osmo-bsc/abis_rsl.c@1961">Patch Set #1, Line 1961:</a> <code style="font-family:monospace,monospace">        chan_counts_t bts_counts;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">we don't tend to use *_t types in osmocom (see kernel programming style guide). […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">well, we usually use 'struct foo' or 'enum foo', but this typedef (used to pinpoint the array sizes) can't be used as 'typedef chan_counts', just 'chan_counts'.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So what I often like to do, name the instance exactly like the type:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  struct foo foo;</pre><p style="white-space: pre-wrap; word-wrap: break-word;">is not possible with a typedef like this.<br>That's why I added the _t. Not harmful, is it?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/abis_rsl.c@1974">Patch Set #1, Line 1974:</a> <code style="font-family:monospace,monospace">        free_tchf = bts_counts[CHAN_COUNTS1_ALL][CHAN_COUNTS2_FREE][GSM_LCHAN_TCH_F];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'd rather prefer getters here, this way the array can be kept internal to its own module and no nee […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">To me it all seems super obvious, could it be about improving readability?<br>Maybe abbreviating CHAN_COUNTS1_ to CC1_ could make it more readable?<br>(but CC also means Call Control ...)<br>CHANS1_ CHANS2_ ?</p><p style="white-space: pre-wrap; word-wrap: break-word;">...you mean a getter like this?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  static inline unsigned int chan_counts_get(chan_counts vals,<br>        enum chan_counts_dim1 dim1, enum chan_counts_dim2 dim2, enum gsm_chan_t dim3)<br>  {<br>       return vals[dim1][dim2][dim3];<br>  }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  free_tchf = chan_counts_get(bts_counts, CHAN_COUNTS1_ALL, CHAN_COUNTS2_FREE, GSM_LCHAN_TCH_F);</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Does that help? From typedef chan_counts_t it's obvious which enum values to use in which array dimension??</p><p style="white-space: pre-wrap; word-wrap: break-word;">"some comment here describing what you fetch here"<br>I fetch literally the count of "ALL FREE TCH_F" as the enum values indicate.<br>Besides ALL, there could be STATIC or DYNAMIC, as seen in the enum definition, but here i get ALL.<br>FREE is obvious, TCH_F is obvious ... what is there to explain?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/bts_trx.c">File src/osmo-bsc/bts_trx.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/osmo-bsc/+/25972/1/src/osmo-bsc/bts_trx.c@327">Patch Set #1, Line 327:</a> <code style="font-family:monospace,monospace">void trx_count_lchans(chan_counts_t ret, const struct gsm_bts_trx *trx)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">All this could go into a new counts.c file, to have all this logic in one place. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">re "all logic in one file" -- who introduced the separation of bts and trx? I dimly remember that this used to be in the same place at some point? The only reason why this is here is because previous bts lchan counting and trx lchan counting code was in these separate places. I assumed that is so for a reason, so rather leave all trx logic in the same file...?</p><p style="white-space: pre-wrap; word-wrap: break-word;">we usually put out-arguments first, right? 'ret' is a returned argument, maybe I should doc that</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972/1/src/osmo-bsc/gsm_data.c">File src/osmo-bsc/gsm_data.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/osmo-bsc/+/25972/1/src/osmo-bsc/gsm_data.c@650">Patch Set #1, Line 650:</a> <code style="font-family:monospace,monospace"> * TODO: include possible VAMOS secondary lchans? */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">what about this TODO?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">wait, this bit of patch may be an unrelated leftover from a previous patch version...</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972">change 25972</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/c/osmo-bsc/+/25972"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bsc </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I2fb48c549186db812b1e9d6b735a92e80f27b8d3 </div>
<div style="display:none"> Gerrit-Change-Number: 25972 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 27 Oct 2021 20:57:08 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>