Thank you for review, comments are inline, rewrite in progress :)
07.04.2013 22:26, Sylvain Munaut пишет:
- Why do you need to add stddef.h ?
Required for size_t.
Also before of spacing change, you're removing an empty line that was there to separate the includes from the function comment.
- wrt to :
+uint16_t osmo_get2bytes(const uint8_t *a); +void osmo_64pack2pbit(uint64_t in, pbit_t *out);
Theses essentially look like integer accessors, one is a read for uint16_t in big endian and the other a store for uint64_t in little endian, but same basic idea. But you're using completely different naming schemes for both ... I think they should reflect that their name should reflect the LE/BE part and the unaligned part as well, I even think there are already macro somewhere for that. I would also add other formats like LE/BE 16/32/64 bits store/load all at once rather than each format when needed. If we add accessort, might as well add all the basic types. And finally those should really be inline functions in the .h, no point of doing a function call for that.
Is there some library I can use for that? It's indeed fairly trivial accessors but I didn't managed to find those in osmocom code.
- In include/osmocom/gsm/gsm_utils.h, for ms_a5n_support you take
only one cm argument ... that means the app needs to know if it should give cm3 or cm2, I would take cm2 and cm3 separately and the app just has to give both. I might also extend ms_cm2_a5n_support and ms_cm3_a5n_support to return -1 in case it couldn't be determined (because the required test isn't in that classmark), but that last point is just a suggestion.
Will fix, thanks.
- TAB vs SPACE indentation !
Doh! Could you remind me - what was the magic tool which takes care of indentation for kernel devs? Osmocom uses linux kernel code style, is it?
- I would split the patch further between CM functions / buffere
reversion / accessorts.
- Is ROL16 really something we exepect to do at a lot of places ?
just asking ... in anycase, also should probably be inline.
I thought it might but it turned out to be used only in single file. Should I hide it away?