<p><a href="https://gerrit.osmocom.org/c/osmo-bsc/+/25972">View Change</a></p><p>4 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@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;">If you don't move them yourself, Pau will do this during his next refactoring ;)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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@1448">Patch Set #1, Line 1448:</a> <code style="font-family:monospace,monospace">typedef unsigned int chan_counts_t [CHAN_COUNTS1_N][CHAN_COUNTS2_N][_GSM_LCHAN_MAX];</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">lol, the linter says "NEW_TYPEDEFS: do not add new typedefs" […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd go for the struct with the multidimensional array inside it. This way it can be easily extended if needed for whatever reason.</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@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;">To me it all seems super obvious, could it be about improving readability? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes please, a getter like you wrote, and with some minimal description like "Remember to call bts_counts_chans() or alike before using it.</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;">re "all logic in one file" -- who introduced the separation of bts and trx? I dimly remember that th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">As I said, I'm fine with keeping it either way, but if you keep it here, let's make it an operation on trx and have trx as first param, since the function is called "trx_count_lchans()". If you want to keep the ret function first, then it'd be clear naming it "count_lchans_trx" and moving it to the count specific file.</p><p style="white-space: pre-wrap; word-wrap: break-word;">This way one has some code which is consistent in some sort of object-oriented way. So ig a function starts with trx_*, then you know it's an operation on trx and the first param is a trx.</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-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 28 Oct 2021 11:47:36 +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: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>