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.org
Patch Set 9:
(8 comments)
I have various cosmetic comments, as inlined. There would be plenty
more, but stopping for now.
https://gerrit.osmocom.org/#/c/416/9/src/decoding.cpp
File src/decoding.cpp:
Line 655: int i, rc;
why move this var declaration? I prefer each var on a separate line, and rather don't mix cosmetic changes with "real" ones.
Line 710: * return what we have so far and assume the
extraneous whitespace change which also breaks proper indenting
Line 714: LOGP(DRLCMACDL, LOGL_DEBUG,
another extraneous whitespace change: don't drop the blank line above the LOGP().
https://gerrit.osmocom.org/#/c/416/9/src/egprs_rlc_compression.cpp
File src/egprs_rlc_compression.cpp:
Line 34: int idx; /* interate index of the code word table */
I don't think you need to comment on these variable names,
they are very obvious as they are.
(btw, it's spelled 'iterator' with o)
https://gerrit.osmocom.org/#/c/416/9/tests/tbf/TbfTest.cpp
File tests/tbf/TbfTest.cpp:
Line 44: #define NUMBER_OF_TEST_CASE 9
nitpick: add S for NUMBER_OF_TEST_CASES,
or actually rather call it TBF_TEST_CASE_COUNT?
Line 61: {0xff, 0xff, 0xff, 0xf8, 0x00, 0x00, 0x01, 0xff, 0xff, 0xff, 0xf8, 0x00,
I would appreciate line breaks and indenting for readability:
add a new-line after each '}' (possibly also after some '{')
and indent to show the level depths; heed <80 chars width.
Line 64: {(int8_t)40, (uint8_t)1, {0x53, 0x06, 0xc5, 0x40, 0x6d}, {0xff, 0xff, 0xff, 0xff, 0xff,
are these casts (int8_t) and (uint8_t) really necessary?
Line 125: * and to verify the result with expected result
(fix whitespace)
--
To view, visit https://gerrit.osmocom.org/416
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieae1992ed4b02bb1e09eec2d3de1a030eabd16ce
Gerrit-PatchSet: 9
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: pravin <pravin.manoharan at radisys.com>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pravin <pravin.manoharan at radisys.com>
Gerrit-HasComments: Yes