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/.
pravin gerrit-no-reply at lists.osmocom.orgPatch Set 24: (26 comments) https://gerrit.osmocom.org/#/c/416/24/src/decoding.cpp File src/decoding.cpp: Line 700: LOGP(DRLCMACDL, LOGL_DEBUG, "Compress bitmap exist, " > 'exists'? Done Line 701: "CRBB LEN =%d and Starting color code =%d", > 'LEN = %d' or 'LEN=%d', same with 'code=%d' Done https://gerrit.osmocom.org/#/c/416/24/src/egprs_rlc_compression.cpp File src/egprs_rlc_compression.cpp: Line 36: /* Function to build the codeword tree > drop the 'Function to', we can see that it's a function. Furthermore, it is Done Line 43: int len; > whitespace: only single spaces please Done Line 50: for (i = 0; i < len; i++) { > single spaces Done Line 55: } else if (cdwd[idx][i] == '1') { > it's only '0' or '1', so a simple else would suffice. Done Line 63: (iter->run_length) = idx; > drop the braces around iter->run_length Done Line 65: (iter->run_length) = (idx - 63) * 64; > This looks odd. The first 64 run lenghts are 0, 1, 2, ..., 63, and the foll Done Line 69: > add a spec paragraph or table reference? Done Line 234: /* search_runlen function will return the runlength for the codeword > We see that it is called search_runlen and that it is a function, there is Done Line 236: * \param bmbuf[in] Recevied compressed bitmap buf > "Received" Done Line 239: * \param rlen[out] Run length value > "Calculated run length" Done Line 259: >>(7-(MOD8(bit_pos)))) & 0x01); > add spaces after '>>' and around the '-'. No need to put (MOD8()) in braces Done Line 274: /* Function to decompress crbb > 'Function to' is obsolete, so is 'decompress crbb' since it merely repeats Done Line 276: * \param clr_code_bit[in] Color code 1 for Ones runlength 0 for Zero runlength > clr generally means 'clear', rather name it 'color_code_bit' instead. Consi Done Line 329: /* init function to build codeword */ > obsolete comment Done https://gerrit.osmocom.org/#/c/416/24/src/egprs_rlc_compression.h File src/egprs_rlc_compression.h: Line 6: > In case below defines are used only in egprs_rlc_compression.c, they should Done Line 7: #define MAX_CDWDTBL_LEN 79 /* total number of codewords */ > since this is not for all thinkable codeword tables, it should have a prefi Done Line 8: #define BITS_TO_BYTES(X) ((X ? (X/8):0)+1) > Make sure to add braces around every X. Here we are passing position of the bit to the macro and expecting the byte number. For example if the bit position is between 0-7 then it should return 1. If the bit position is between 8-15 it should return 2 and so on. Line 9: #define MOD8(X) (((X)+8) & (0x07)) > mod 8 is simply '((X) & 0x7)' -- what is the +8 for?? Done Line 11: typedef struct node { > this struct could be opaque, with only a 'typedef struct <name>' here, and I will change the structure instance name as you suggested. In normally all structures in osmo-pcu files are defined in .h file itself eg:gsm_rlcmac.h. Line 17: int decompress_crbb(int8_t compress_bmap_len, uint8_t clr_code_bit, > I'd make this a static member function of class egprs_compress Done Line 20: /* Creating singleton class*/ > whitespace: 'class */' Done Line 23: public: > actually, decompress_crbb could be the *only* public member of this class ( Compression support will be added in next patch. Line 41: if (decode_tree_init() < 0) { > decode_tree_init() *always* returns zero. Done Line 48: /* Private because nobody is allow to delete on object of this type */ > I'd prefer Done -- 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: 24 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