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 25:
(25 comments)
https://gerrit.osmocom.org/#/c/416/25/src/decoding.cpp
File src/decoding.cpp:
Line 697: LOGP(DRLCMACDL, LOGL_DEBUG, "Compress bitmap exists, "
Actually ... what do you mean by 'exists'? Is received?
https://gerrit.osmocom.org/#/c/416/25/src/egprs_rlc_compression.cpp
File src/egprs_rlc_compression.cpp:
Line 22: extern const char *zero_run_len_code_list[EGPRS_CODEWORDS];
the two arrays are not extern but defined further below.
Line 37: /* The function expands the given tree by incorporating
Still no need to state that it is a function! For example:
'Expand the given tree...'
Line 40: * \param cdwd[in] Code word
plural, and indicate the length:
'Array of code words, length must be EGPRS_CODEWORDS'
Line 74:
(remove space from the blank line)
Line 248: * \param rlen[out] Caluculate run length
fix typo. As I said before, it should say 'Calculated', not 'Calculate' (which would mean something else entirely).
Line 267: dir = ((bmbuf[BITS_TO_BYTES(bit_pos) - 1]
Come on, in BITS_TO_BYTES you '+1' and here you '-1' again? Since this is the only place using that define, that amounts to pure code bloat. This also completely invalidates your previous comment on the calculation. Drop the define entirely and just do / 8 here.
Line 268: >> (7 - MOD8(bit_pos))) & 0x01);
Drop outermost braces.
Line 271: if (((dir&0x01) == 0) && (iter->left != NULL))
dir is already & 0x01 above.
Since dir is boolean, no need to == 0:
'if (!dir && (iter...'
Line 273: else if (((dir&0x01) == 1) && (iter->right != NULL))
'if (dir && (iter...'
Line 279: (*rlen) = (iter->run_length);
drop all braces
Line 283: /* Function to decompress compressed received block bitmap
I *see* that it is a function! Start with
"Decompress ..."
Drop the 'compressed', if you're decompressing it is obvious that it is compressed data.
Line 285: * \param color_code_bit[in] Color code 1 for Ones runlength 0 for Zero runlength
As asked before, please explain why you call it 'color code' in the first place.
Line 297: uint8_t data = 0x0;
'= 0' ?
Line 304: egprs_compress *compress = egprs_compress::instance();
just ' = instance()' should suffice?
Line 307: if (color_code_bit == 1) {
'if (color_code_bit) {'
Line 320: color_code_bit ? color_code_bit = 0 : color_code_bit = 1;
"color_code_bit = !color_code_bit;"
It is a bool now.
https://gerrit.osmocom.org/#/c/416/25/src/egprs_rlc_compression.h
File src/egprs_rlc_compression.h:
Line 7: #define BITS_TO_BYTES(X) (((X) ? (X)/8 : 0) + 1)
Then this should be called BITPOS_TO_BYTES instead.
This is used only in egprs_rlc_compression.cpp, so move it there.
Line 8: #define MOD8(X) ((X) & (0x07))
you're using this define only once, just drop it and use '& 0x7' directly instead.
Line 14: } egprs_compress_node;
What I meant: here just say "struct egprs_compress_node;",
move the definition to egprs_rlc_compression.cpp.
Furthermore, since this is C++, you don't need the 'typedef' shim at all.
Line 32: {
I would move the function definition to .cpp as well
Line 41: decode_tree_init();
and move this one to .cpp as well.
The idea is to have the public API as lean as possible.
https://gerrit.osmocom.org/#/c/416/25/tests/bitcomp/BitcompTest.cpp
File tests/bitcomp/BitcompTest.cpp:
Line 124: int check_result(bitvec bits, uint8_t *exp_data, int exp_len)
cur_bit is unsigned, so should be exp_len (avoid compiler warning with -Wall)
Line 150: int itr;
make itr unsigned to avoid compiler warning with -Wall
Line 164: (test[itr].crbb_len + 7)/8), test[itr].crbb_len
You're using 'test[itr]' over and over, rather define a reference (pointer or C++ reference) and use that.
--
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: 25
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: arvind.sirsikar <arvind.sirsikar at radisys.com>
Gerrit-Reviewer: pravin <pravin.manoharan at radisys.com>
Gerrit-HasComments: Yes