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 17:
(23 comments)
https://gerrit.osmocom.org/#/c/642/17//COMMIT_MSG
Commit Message:
Line 9: In this commit two modules were added:
up for opinions, but it's usually shortest to write the
imperative present tense form, which I personally like
very much for its crisp brevity:
"Add compression control"
and
"Add two modules:"
and
"gprs_sndcp_comp.h/c: Handle creation and destruction..."
Line 12: destruction of conpression entites. It handles the allocation
conpression :)
Line 21: It works, but is not yet ready for merging. Both files depend
writing this in a commit log that is up for review doesn't really make sense -- or rather, once the patch is ready for merging, this sentence must definitely go away.
https://gerrit.osmocom.org/#/c/642/17/openbsc/include/openbsc/gprs_sndcp_comp.h
File openbsc/include/openbsc/gprs_sndcp_comp.h:
Line 3: /* (C) 2016 by Sysmocom s.f.m.c. GmbH
I think it would be good to add <info at sysocom.de>.
See gprs/gtphub.h.
So far we have 'sysmocom' in lower case only.
Not sure if it matters, but can't hurt to keep it so.
Line 24: #define _GPRS_SNDCP_COMP_H
Older files use this way, yes, but in newer files we now
tend to use
#pragma once
instead. This allows dropping all of #ifndef, #define and #endif for one short line. See for example gprs/gtphub.h.
Line 60: llist_head
rather not break an argument over multiple lines.
Remove an indent if necessary. (same below)
https://gerrit.osmocom.org/#/c/642/17/openbsc/include/openbsc/gprs_sndcp_pcomp.h
File openbsc/include/openbsc/gprs_sndcp_pcomp.h:
Line 3: /* (C) 2016 by Sysmocom s.f.m.c. GmbH
"sysmocom" (?)
and email addr
Line 24: #define _GPRS_SNDCP_pcomp_H
#pragma once
https://gerrit.osmocom.org/#/c/642/17/openbsc/include/openbsc/sgsn.h
File openbsc/include/openbsc/sgsn.h:
Line 95: } pcomp_rfc1144;
curious, any reason to not add this at the end of the struct?
https://gerrit.osmocom.org/#/c/642/17/openbsc/src/gprs/gprs_sndcp_comp.c
File openbsc/src/gprs/gprs_sndcp_comp.c:
Line 40: *ctx, const struct
(line breaks)
Line 59: comp_entity->nsapi_len * sizeof(int));
sizeof(struct member)
Line 83: OSMO_ASSERT(NULL);
OSMO_ASSERT(0);
Line 114: "Header compression entity (%i) creation failed!\n",
we usually use %d, not %i (same below)
Line 173: if (comp_entity_to_delete) {
exit early and save indent space:
if (!comp_entity_to_delete)
return;
Line 190:
remove extra blank line before '}' (more like this below)
Line 196: llist_head
(line breaks, more below)
Line 213: if (comp_entity) {
if (!comp_entity)
return NULL;
Line 249: OSMO_ASSERT(comp_entities);
does any code path ever even handle a NULL comp_entities?
It gives a warm fuzzy feeling to assert on this, but
usually this is not necessary...
https://gerrit.osmocom.org/#/c/642/17/openbsc/src/gprs/gprs_sndcp_pcomp.c
File openbsc/src/gprs/gprs_sndcp_pcomp.c:
Line 118: /* Note: This function is automatically called from
apparently we want the comment style like
/*
* text
* more text
*/
Line 135: OSMO_ASSERT(false);
you're not including <stdbool.h> ... OSMO_ASSERT(0) again?
Line 167: uint8_t *comp_ptr; /* Not used */
then why have it?
Line 174: compr_len = slhc_compress(comp, data_i, len, data_o, &comp_ptr, 0);
it is used here?
Line 219:
drop blank line?
--
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: 17
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