This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgPatch Set 13: (9 comments) I have only cosmetic comments, but their number has some weight. I wouldn't block the commit on these, but would be nice-to-have. Some are definitely "wrong" while others are just my opinion. https://gerrit.osmocom.org/#/c/655/13/src/pcu_vty.c File src/pcu_vty.c: Line 488: cfg_pcu_dl_arq_cmd, the other DEFUN() in this file indent the arguments to line up with the first argument, i.e. 6 spaces instead of a tab. https://gerrit.osmocom.org/#/c/655/13/src/rlc.h File src/rlc.h: Line 113: enum egprs_rlcmac_dl_spb_values { an enum always contains "values", so this name could be shortened by dropping the "_values" part. https://gerrit.osmocom.org/#/c/655/13/src/tbf.h File src/tbf.h: Line 424: enum egprs_rlc_dl_reseg_bsn_state egprs_dl_get_data_buffer data is always in a buffer, maybe drop the "_buffer" https://gerrit.osmocom.org/#/c/655/13/src/tbf_dl.cpp File src/tbf_dl.cpp: Line 383: bts->bts_data()->dl_arq_type); I would add another tab indent for the arguments of get_retx_mcs() Line 392: bts->bts_data()->dl_arq_type, bsn); I would add another 5-spaces indent for the arguments of LOGP(). If the 80 characters width is too tight in this nested if(), it would be good to move this code block to a new function. Line 660: EGPRS_RESEG_SECOND_SEG_SENT)) I find the indenting of the if condition confusing. IMHO this would improve readability: if (get_egprs_dl_spb_status(index) == EGPRS_RESEG_DL_DEFAULT || get_egprs_dl_spb_status(index) == EGPRS_RESEG_SECOND_SEG_SENT) The rest of this file does not use braces around '==' conditionals. Line 1292: (const int bsn) if you also drop the "_value" from "get_egprs_dl_spb_value", this will fit on one line. https://gerrit.osmocom.org/#/c/655/13/tests/tbf/TbfTest.cpp File tests/tbf/TbfTest.cpp: Line 1839: if ((mcs == 6) && (demanded_mcs == 3)) { basically the entire function is in an if {} block. Remove the indent by doing if (!(mcs == 6 && demanded_mcs == 3)) return; here ... unless you expect more else if() branches to be added later? Line 1937: ((mcs == 4) && (demanded_mcs == 1))) { again, remove indent by if (![...]) return; -- To view, visit https://gerrit.osmocom.org/655 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I242afdd8ae7622dec8593b26382ad66bad5b9516 Gerrit-PatchSet: 13 Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Owner: arvind.sirsikar <arvind.sirsikar at radisys.com> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Holger Freyther <holger at freyther.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-Reviewer: arvind.sirsikar <arvind.sirsikar at radisys.com> Gerrit-HasComments: Yes