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/.
Holger Freyther gerrit-no-reply at lists.osmocom.orgPatch Set 15: (19 comments) https://gerrit.osmocom.org/#/c/416/15/src/egprs_rlc_compression.cpp File src/egprs_rlc_compression.cpp: Line 45: if (iter->left == NULL) we tend to use if (iter->left) Line 54: if (iter != NULL) { if (!iter) Line 229: Node *root, /* root of Ones or Zeros tree */ same comment as before.. very unusual and not helpful like this. Use @param syntax Line 242: if ((iter->left == NULL) && (iter->right == NULL)) if (iter->left && ..) Line 290: /*If run length > 64, need makeup and terminating code*/ spacing https://gerrit.osmocom.org/#/c/416/15/src/egprs_rlc_compression.h File src/egprs_rlc_compression.h: Line 39: { What is the reason to leave tall_tree_ctx completely uninitiaized in the constructor? Most likely you should do decode_tree_init as well to construct the tree on first usage (and still do the call from main to have it ready) Line 43: void free_codeword(Node *root); remove, there is no clean-up of that. No need to have a partially working function.. Line 44: ~egprs_compress() We can't use c++11 yet just leave it private and don't implement it. If somebody still tries to delete it.. stuff will go wrong. Line 49: static Node *ones_list; you don't need that to be static. Line 52: int decode_tree_init(void *tall_pcu_ctx) do not shadow "tall_pcu_ctx" what is the point of having this inline? It is a one time init function. Line 59: zeros_list = talloc(tall_tree_ctx, Node); ise create_tree_node? Line 60: instance()->build_codeword( you have a "this" and we do not have concurrency here. So no reason to use instance() here. https://gerrit.osmocom.org/#/c/416/15/tests/Makefile.am File tests/Makefile.am: Line 32: $(LIBOSMOGB_LIBS) \ Build error: libosmogb has an undefined symbol it calls. You can either decide not to link libosmogb (why would a T4 decoder need GSM+GB anyway)? or define the symbol so it links https://gerrit.osmocom.org/#/c/416/15/tests/bitcomp/BitcompTest.cpp File tests/bitcomp/BitcompTest.cpp: Line 25: void *tall_pcu_ctx; give it some room to breath and add a newline Line 35: {67, 1, Use C99 initializers Line 103: if (bits.cur_bit != exp_len) So.. maybe print the data in the error case so somebody debugging has it more easy to see what is going on? Line 138: decompress_crbb(test[itr].crbb_len, test[itr].cc, Result ignored Line 144: if (test[itr].verify) { Invalid data not checking the result. That post-condition is too weak. Check the result with an expected result Line 174: extern "C" { if you link less, you might be able to drop these. -- 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: 15 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