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 23:
(17 comments)
quite a large patch, can't get through all of it ATM...
https://gerrit.osmocom.org/#/c/642/23/openbsc/include/openbsc/gprs_llc.h
File openbsc/include/openbsc/gprs_llc.h:
Line 179: /* In this two list_heads we will store the
"these two"
https://gerrit.osmocom.org/#/c/642/23/openbsc/include/openbsc/gprs_sndcp_comp.h
File openbsc/include/openbsc/gprs_sndcp_comp.h:
Line 33: unsigned int entity; /* see also: 6.5.1.1.3 and 6.6.1.1.3 */
we usually put comments above the members, not behind it.
See for example:
openbsc/include/openbsc/bsc_api.h
openbsc/include/openbsc/gprs_llc.h
openbsc/src/gprs/gprs_sndcp.h
And actually, I would have added this header file in
openbsc/include/openbsc/
and am surprised to find a gprs_sndcp.h in src/gprs/.
Having headers in openbsc/include/openbsc allows including this
header with '#include <openbsc/gprs_sndcp_comp.h>', always.
I guess we should also move gprs_sndcp.h to include/openbsc?
Line 64: llist_head *comp_entities,
line breaks in the middle of parameters again...
https://gerrit.osmocom.org/#/c/642/23/openbsc/include/openbsc/gprs_sndcp_pcomp.h
File openbsc/include/openbsc/gprs_sndcp_pcomp.h:
Line 30: const struct gprs_sndcp_comp_field *comp_field);
if you have tabs and spaces mixed like here, line
up after the '(' column.
(I think we also have three-tabs and not lining up somewhere)
https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/gprs_llc.c
File openbsc/src/gprs/gprs_llc.c:
Line 150: xid_field_request_l3 = xid_field_request;
Could also use llist_for_each_entry_reverse() with a 'break;'
in the if {} to pick the last one.
https://gerrit.osmocom.org/#/c/642/23/openbsc/src/gprs/gprs_sndcp.c
File openbsc/src/gprs/gprs_sndcp.c:
Line 875: /* Comile bytestream */
"Compile"
Line 895: sizeof
rather remove one tab indent and keep sizeof() in one line
Line 913: /* Hanle header compression entites */
"Handle"
Line 917: /* Note: This functions also transforms the comp_field into its
english nitpicking...
"This function"? "These functions"?
"transform ... to" (not into).
End the sentence before "The processed"
Line 937: "Rejecting RFC1144 header conpression...\n");
"compression"
Line 947: "Rejecting RFC2507 header conpression...\n");
"compression"
Line 956: "Rejecting ROHC header conpression...\n");
"compression"
Line 970: /* Note: This functions also transforms the comp_field into its
s.a. -- do you really want the same comment twice?
Line 984: "Rejecting V42BIS data conpression...\n");
"compression"
Line 992: LOGP(DSNDCP, LOGL_DEBUG, "Rejecting V44 data conpression...\n");
"compression"
Line 1133: xid_field_confirmation->
weird line break
Line 1139: "Modified version of received SNDCP-XID received from the phone:\n");
only one "received"?
--
To view, visit https://gerrit.osmocom.org/642
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia00260dc09978844c2865957b4d43000b78b5e43
Gerrit-PatchSet: 23
Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes