[PATCH 1/3] Add generic LE/BE load/store uint type convertors and use them in msgb

[Sylvain Munaut]

Hi Max,

>
>  src/crc*gen.c
>  include/osmocom/core/crc*gen.h
> -
> +include/osmocom/core/bit*gen.h

When inserting new line, I think trying to keep them alphabetically
sorted by group is a nice touch.
It doesn't matter here and no real need to change it, but it's just as
a general remark for future patches or changes.


>


> +/*! \brief load unaligned n-byte integer (little-endian encoding) into uintXX_t */
> +static inline uintXX_t osmo_loadXXle_ext(const void *p, uint8_t n)

When I said use Doxygen style, I mean the complete doc ... with param
& returns and everything.

If you want example of good doc, look at osmo-gmr ( eg, a random file
http://git.osmocom.org/osmo-gmr/tree/src/sdr/pi4cxpsk.c ). Every
function has full doc. For libosmocore or other projects it's not the
case because of legacy code, but it is my strong belief that any
addition of new code should adhere to the strictest standard to try
and converge to a higer code quality and better coverage of the
documentation.

I know it's a pain, but if you do this from the beginning as something
"automatic" like a reflex when writing the code in the first place, it
doesn't take up all that much time.


> +{
> +       uint8_t i;
> +       uintXX_t r = 0;
> +       const uint8_t *q = (uint8_t *)p;
> +       for(i = 0; i < n; r |= ((uintXX_t)q[i] << (8 * i)), i++);
> +       return r;
> +}

Any particular reason you went with this completely generic approach
rather than just handling the few cases manually ? There is only a few
of them.

This is purely informational. I've confirmed that with -O2 the code is
'decent'. and with -O3 it's completely equivalent to hardcoded shift
like they were before in msgb.


>  #include <stdint.h>
> -
> +#include <stddef.h>
> +#include <osmocom/core/bit16gen.h>
> +#include <osmocom/core/bit32gen.h>
> +#include <osmocom/core/bit64gen.h>
>  /*! \defgroup bits soft, unpacked and packed bits
>   *  @{
>   */

Try not to change the original spacing without reason.

Here there was a space after the include and the start of the code to
make things more readable and less "compressed".

Something like :

----
#include <stddef.h>
#include <stdint.h>
#include <osmocom/core/bit16gen.h>
#include <osmocom/core/bit32gen.h>
#include <osmocom/core/bit64gen.h>

/*! \defgroup bits soft, unpacked and packed bits
 *  @{
 */
-----


Cheers,

     Sylvain