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.org
Patch 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