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 4:
(35 comments)
I can't claim to understand in depth, so I have mainly style remarks.
I'm not sure to what extent this is copying code over from another repos,
in which case it might make sense to keep things similar rather than
fix cosmetic issues. These remarks are just what catches my eye and
strikes me as unusual for osmo* coding style.
The commit log reads like this adds the generated code, yet in the
patch it does look like the sources for generating are also included.
It would be good to clarify that. (related: gsm6* files)
https://gerrit.osmocom.org/#/c/816/4/.gitignore
File .gitignore:
Line 113: src/gsm/gsm6*.c
gsm6? I thought this is about gsm05*?
https://gerrit.osmocom.org/#/c/816/4//COMMIT_MSG
Commit Message:
Line 12: beeter to clean up the OsmoBTS source code, sharing this code.
Better than what?
Do you mean to say: "Having this code in libosmocore allows
cleaning up the OsmoBTS source code." ?
Line 28: in generated state.
The general opinion about generated codes was to rather not
even have them in git. Does this copy code from osmo-bts so
that the sources to generate are in osmo-bts, but the generated
code is in libosmocore? That would be rather unfortunate,
IMHO the generating code should move along.
https://gerrit.osmocom.org/#/c/816/4/include/osmocom/gsm/gsm0503.h
File include/osmocom/gsm/gsm0503.h:
Line 29: /*! \file gsm0503.h
This is an unrelated cosmetic fix, it should rather be in a separate commit.
https://gerrit.osmocom.org/#/c/816/4/include/osmocom/gsm/gsm0503_coding.h
File include/osmocom/gsm/gsm0503_coding.h:
Line 1: /*
(one space too many)
Line 6: * (C) 2016 by Tom Tsou <tom.tsou at ettus.com>
we usually have the copyright and license info only in the .c files...
Line 46: int gsm0503_xcch_encode(ubit_t *bursts, uint8_t *l2_data);
you're using stdint.h types, yet this header file has no #includes at all. It would be better to include definitions needed by this header explicitly.
Where is ubit_t coming from?
Line 78: int codec_mode_req, uint8_t *codec, int codecs, uint8_t *ft,
(extra space)
https://gerrit.osmocom.org/#/c/816/4/include/osmocom/gsm/gsm0503_interleaving.h
File include/osmocom/gsm/gsm0503_interleaving.h:
Line 1: /*
whitespace, copyright in .c
Line 27: void gsm0503_xcch_deinterleave(sbit_t *cB, const sbit_t *iB);
missing #include(s)
https://gerrit.osmocom.org/#/c/816/4/include/osmocom/gsm/gsm0503_mapping.h
File include/osmocom/gsm/gsm0503_mapping.h:
Line 1: /*
whitespace, copyright in .c
Line 27: void gsm0503_xcch_burst_unmap(sbit_t *iB, const sbit_t *eB,
#include
https://gerrit.osmocom.org/#/c/816/4/include/osmocom/gsm/gsm0503_parity.h
File include/osmocom/gsm/gsm0503_parity.h:
Line 1: /*
...
https://gerrit.osmocom.org/#/c/816/4/include/osmocom/gsm/gsm0503_tables.h
File include/osmocom/gsm/gsm0503_tables.h:
Line 27: extern const ubit_t gsm0503_pdtch_hl_hn_ubit[4][8];
#include
https://gerrit.osmocom.org/#/c/816/4/src/gsm/Makefile.am
File src/gsm/Makefile.am:
Line 21: gsm610.c gsm620.c gsm660.c gsm0503_conv.c \
Is the gsm610.c in src/codec related to these?
Line 30:
don't add a blank line here:
don't separate libgsmint_*, and even if, don't add unrelated
whitespace changes in the same commit.
Line 42: $(AM_V_GEN)python2 ../../utils/conv_gen.py
for 'make distcheck': rather use $(top_srcdir)/... ?
(same below)
https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_coding.c
File src/gsm/gsm0503_coding.c:
Line 1: /*
whitespace
https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_conv_edge.c
File src/gsm/gsm0503_conv_edge.c:
Line 62:
one blank line too many -- but if the file is moving from
somewhere else probably rather keep it...
https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_interleaving.c
File src/gsm/gsm0503_interleaving.c:
Line 1: /*
whitespace
Line 304:
(two blank lines, same as before)
https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_mapping.c
File src/gsm/gsm0503_mapping.c:
Line 1: /*
ws
Line 29: sbit_t *hl, sbit_t *hn)
this file mixes single-tab indent and aligned indent styles.
would be better to stick to one of them.
https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_parity.c
File src/gsm/gsm0503_parity.c:
Line 1: /*
ws
Line 33:
would be nicer to not have a blank line between comment-for-struct and struct
Line 145:
drop trailing blank line
https://gerrit.osmocom.org/#/c/816/4/src/gsm/gsm0503_tables.c
File src/gsm/gsm0503_tables.c:
Line 1: /*
ws
https://gerrit.osmocom.org/#/c/816/4/utils/conv_gen.py
File utils/conv_gen.py:
Line 30: name = "call-me", description = False, puncture = []):
IMHO pretty weird indenting choice.
I'd have these fixes in a separate commit.
lol: "LOL" :)
Line 53: raise ValueError("Bad polynoms: "
The proper English term would be polynomial / polynomials.
I'd have these fixes in a separate commit.
Line 59: "in a recursive code")
separate commit.
Line 98: # No choice ... (systematic output in recursive case)
separate commit
Line 135: def _print_term(self, fi, num_states, pack = False):
fix whitespace in a separate commit
Line 247:
move combine() to self.combine() also in a separate commit
Line 260: ( G1, 1 ),
cosmetics in a separate comit
(more below)
Line 298: 15, 19, 23, 27, 31, 35, 43, 47, 51, 55, 59, 63, 67, 71,
I hope this is correct :)
looks like pure magic, the numbers change weirdly.
--
To view, visit https://gerrit.osmocom.org/816
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I257a5d015798ee9e690fd035ca97fd971cf9f60a
Gerrit-PatchSet: 4
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes