<p style="white-space: pre-wrap; word-wrap: break-word;">Found some minor things, otherwise +1. Well done, Pau!</p><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/src/bts.h">File src/bts.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-pcu/+/22196/6/src/bts.h@199">Patch Set #6, Line 199:</a> <code style="font-family:monospace,monospace"> uint16_t mcs_mask; /* Allowed MCS mask from struct gprs_rlcmac_bts */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"from BTS" -> "from struct gprs_rlcmac_bts" looks like unintentional replacement. Or if not, what do you mean with 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-pcu/+/22196/6/src/bts.h@227">Patch Set #6, Line 227:</a> <code style="font-family:monospace,monospace"> // BTS:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like this "// BTS:" comment separates old C and C++ code. Did you intend to leave that in? (if so: different comment style, make it more descriptive?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/src/bts.h@264">Patch Set #6, Line 264:</a> <code style="font-family:monospace,monospace">void bts_set_current_frame_number(struct gprs_rlcmac_bts *bts, int frame_number);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What about the 'TODO: change the number to unsigned' comment that was removed?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/src/bts.h@271">Patch Set #6, Line 271:</a> <code style="font-family:monospace,monospace">int bts_tfi_find_free(const struct gprs_rlcmac_bts *bts, enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, int8_t use_trx);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(line > 120 characters)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/src/bts.cpp">File src/bts.cpp:</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-pcu/+/22196/6/src/bts.cpp@569">Patch Set #6, Line 569:</a> <code style="font-family:monospace,monospace">int bts_tfi_find_free(const struct gprs_rlcmac_bts *bts, enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, int8_t use_trx)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(line too long)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/src/pdch.cpp">File src/pdch.cpp:</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-pcu/+/22196/6/src/pdch.cpp@116">Patch Set #6, Line 116:</a> <code style="font-family:monospace,monospace">static inline void sched_ul_ass_or_rej(struct gprs_rlcmac_bts *bts, gprs_rlcmac_bts *bts_data, struct gprs_rlcmac_dl_tbf *tbf)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(long line)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/src/pdch.cpp@708">Patch Set #6, Line 708:</a> <code style="font-family:monospace,monospace"> bts_send_gsmtap_meas(bts(), PCU_GSMTAP_C_UL_CTRL, true, trx_no(), ts_no, GSMTAP_CHANNEL_PACCH, fn, data, data_len, meas);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(while add it, add line breaks in both lines?)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/tests/app_info/AppInfoTest.cpp">File tests/app_info/AppInfoTest.cpp:</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-pcu/+/22196/6/tests/app_info/AppInfoTest.cpp@158">Patch Set #6, Line 158:</a> <code style="font-family:monospace,monospace"> /* FIXME: talloc report disabled, because bts_alloc_ms(bts, ) in prepare_bts_with_two_dl_tbf_subscr() causes leak */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(long line)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196/6/tests/tbf/TbfTest.cpp">File tests/tbf/TbfTest.cpp:</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-pcu/+/22196/6/tests/tbf/TbfTest.cpp@2206">Patch Set #6, Line 2206:</a> <code style="font-family:monospace,monospace"> bts = bts;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">bts = bts ? ;)</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><br> $ git grep -n 'bts = bts;'<br> ...<br> tests/alloc/AllocTest.cpp:467: bts = bts;<br> tests/app_info/AppInfoTest.cpp:86: bts = bts;<br> tests/edge/EdgeTest.cpp:1158: bts = bts;<br> tests/tbf/TbfTest.cpp:487: bts = bts;<br> tests/tbf/TbfTest.cpp:533: bts = bts;<br> tests/tbf/TbfTest.cpp:692: bts = bts;<br> tests/tbf/TbfTest.cpp:838: bts = bts;<br> tests/tbf/TbfTest.cpp:1272: bts = bts;<br> tests/tbf/TbfTest.cpp:1587: bts = bts;<br> tests/tbf/TbfTest.cpp:2206: bts = bts;<br> tests/tbf/TbfTest.cpp:2271: bts = bts;<br> tests/tbf/TbfTest.cpp:2318: bts = bts;<br> tests/tbf/TbfTest.cpp:2401: bts = bts;</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/22196">change 22196</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-pcu/+/22196"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I7d12c896c5ded659ca9d3bff4cf3a3fc857db9dd </div>
<div style="display:none"> Gerrit-Change-Number: 22196 </div>
<div style="display:none"> Gerrit-PatchSet: 6 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 19 Jan 2021 14:40:52 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>