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/.
Harald Welte gerrit-no-reply at lists.osmocom.org
Patch Set 5: Code-Review-1
(9 comments)
https://gerrit.osmocom.org/#/c/641/5/openbsc/include/openbsc/gprs_sndcp_xid.h
File openbsc/include/openbsc/gprs_sndcp_xid.h:
PS5, Line 74: = 2
the compiler would just hand out the next number in the enum, no need for the '= 2' explicitly. same for rfc2507_pcomp below.
Line 106: int max_cid; /* (default 15) */
can those values become negative? your patch v1 stated 'unsigned int' for many values, which now have become 'int'.
https://gerrit.osmocom.org/#/c/641/5/openbsc/src/gprs/gprs_sndcp_xid.c
File openbsc/src/gprs/gprs_sndcp_xid.c:
Line 60: /*
please avoid wasting two additioanl lines for the start and the end of comments '/*' and '*/'. Also, comments regarding function arguments are best placed above the function, where its purpose is described. And if you want to be super future-compatible, you might at the same time already use doxygen syntax, which we so far only use in libraries but not yet in applications.
PS5, Line 94: ETSI TS 144 065
all other code references the 3GPP spec number, not the ETSI. Please follow existing style.
Line 1494: /* Parse and add comp_field */
way too deep indenting, looses effective line length and makes code more complex to read than needed. Easy fix: early 'continue' like:
if (tag != SNDCP_XID_PROTOCOL_COMPRESSION && tag != SNDCP_XID_DATA_COMPRESSION)
continue;
then you have saved already one level of indent. If there are no other optiosn like this, split the function in multiple functions.
PS5, Line 1539: (
extra parenthsis. Where's the advantage over "if (!comp_fields)" ? Same in other if statements below.
Line 1546: sizeof(struct gprs_sndcp_hdrcomp_entity_algo_table));
always use sizeof(*lt) here. Advantages:
* statement fits on one line
* less to type
* if we ever change the type of 'lt', the memset/memcpy/etc. will continue to work without change.
Line 1726: if ((!comp_fields_incomplete) || (!comp_fields))
again the extra paranthesis which I don't understand.
PS5, Line 1822: LOGL_DEBUG
90% of your function still uses LOGL_DEBUG, not logl.
--
To view, visit https://gerrit.osmocom.org/641
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If2d63fe2550864cafef3156b1dc0629037c49c1e
Gerrit-PatchSet: 5
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-HasComments: Yes