6
\$\begingroup\$

I recently programmed some byte swapping functions. As a special I did a function which can convert an entire int array. I please you to look at the following aspects:

  • portability
  • performance
  • usability
  • efficiency
#include <stdint.h>

uint16_t _bswap16(uint16_t a)
{
  a = ((a & 0x00FF) << 8) | ((a & 0xFF00) >> 8);
  return a;
}

uint32_t _bswap32(uint32_t a)
{
  a = ((a & 0x000000FF) << 24) |
      ((a & 0x0000FF00) <<  8) |
      ((a & 0x00FF0000) >>  8) |
      ((a & 0xFF000000) >> 24);
  return a;
}

uint64_t _bswap64(uint64_t a)
{
  a = ((a & 0x00000000000000FFULL) << 56) | 
      ((a & 0x000000000000FF00ULL) << 40) | 
      ((a & 0x0000000000FF0000ULL) << 24) | 
      ((a & 0x00000000FF000000ULL) <<  8) | 
      ((a & 0x000000FF00000000ULL) >>  8) | 
      ((a & 0x0000FF0000000000ULL) >> 24) | 
      ((a & 0x00FF000000000000ULL) >> 40) | 
      ((a & 0xFF00000000000000ULL) >> 56);
  return a;
}

Byte Swapping functions for arrays:

#include <stdint.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>


void *_bswap_a(void *restrict const array, size_t const elem_size, size_t const ndimensions, size_t const *restrict const dimensions, bool const copy, void (*bswapfcn)(void *))
{
    /* Validate arguments */
    if (array == NULL || elem_size <= 1 || ndimensions == 0 || dimensions == NULL || bswapfcn == NULL) {
        return NULL;
    }

    {
        size_t array_size       = elem_size;
        size_t i                = 0;
        unsigned char *a        = array;
        // I know, this is only needed for copy=true....
        unsigned char *elemval  = malloc(elem_size);

        if (!elemval) return NULL;

        /* Caluclate array size */
        for (i = 0; i < ndimensions; ++i) {
            array_size *= dimensions[i];
        }

        /* Copy the array ? */
        if (copy) {
            a = malloc(array_size);

            if (!a) {
                free(elemval);

                return NULL;
            }
        }

        /* Go through each element */
        for (i = 0; i < array_size; i += elem_size) {
            /* Pointer to element */
            void *element   = (((unsigned char *)array) + i);

            if (copy) {
                memcpy(elemval, element, elem_size);

                bswapfcn(elemval);

                memcpy(a + i, elemval, elem_size);
            } else {
                // Swap the bytes
                bswapfcn(element);
            }
        }

        free(elemval);

        return a;
    }
}


void _bswap16f(void *el)
{
    uint16_t    *ptr    = el;
    uint16_t    val     = *ptr;

    *ptr = _bswap16(val);
}

uint16_t *_bswap16a(uint16_t *restrict a, size_t const ndimensions, size_t *restrict const dimensions, bool const copy)
{
    return _bswap_a(a, sizeof(uint16_t), ndimensions, dimensions, copy, _bswap16f);
}

void _bswap32f(void *el)
{
    uint32_t    *ptr    = el;
    uint32_t    val     = *ptr;

    *ptr = _bswap32(val);
}

uint32_t *_bswap32a(uint32_t *restrict const a, size_t const ndimensions, size_t *restrict const dimensions, bool const copy)
{
    return _bswap_a(a, sizeof(uint32_t), ndimensions, dimensions, copy, _bswap32f);
}

void _bswap64f(void *el)
{
    uint64_t    *ptr    = el;
    uint64_t    val     = *ptr;

    *ptr = _bswap64(val);
}

uint32_t *_bswap64a(uint64_t *restrict const a, size_t const ndimensions, size_t *restrict const dimensions, bool const copy)
{
    return _bswap_a(a, sizeof(uint64_t), ndimensions, dimensions, copy, _bswap64f);
}

Usage:

int main(int argc, const char **argv)
{
    /* Swap 32bit Integer */
    uint32_t _swappedInteger            = _bswap32(32);
    /* Swap 32bit Integer array (2 dimensional) and copy the array */
    uint32_t _2darray[2][4]             = {{8, 7, 6, 5}, {4, 3, 2, 1}};
    size_t dimensions[2]                = {2, 4};
    uint32_t (*_swappedArray)[4]        = (void *)_bswap32a((void *)_2darray, 2, dimensions, true);

    fprintf(stderr, "%-12u:%4u \n", _swappedInteger, _bswap32(_swappedInteger));
    fprintf(stderr, "%-12u:%4u \n", _swappedArray[1][2], _bswap32(_swappedArray[1][2]));

    /* _swappedArray is a copy of _2darray so we need to free it */
    free(_swappedArray);
}
\$\endgroup\$
3
\$\begingroup\$
  1. Regarding performance: Well this will reduce portability but MSVC, gcc and clang (same as gcc I think) all have intrinsics defined for bswap which will probably perform better than your implementations. At least on x86 they'll probably make use of the bswap instruction. So you might want to have a few #ifdef'ed alternatives for different compilers.

  2. You are allocating memory which is only needed for a specific code path so maybe you should just not allocate it if copy == false. You can set elemVal = NULL and still unconditionally call free on it as free(NULL) is perfectly legal.

  3. Your naming is a bit terse. bswap itself is ok as it's a reasonably known operation but what the heck is _bswap_a and _bswapf (it's also inconsistent _a vs f)? Please prefer more explanatory names like _bswap_array and _bswap_field.

  4. I find code like this confusing to read (the pointer alias is confusing to me):

       void _bswap16f(void *el)
    {
        uint16_t    *ptr    = el;
        uint16_t    val     = *ptr;
        *ptr = _bswap16(val);
    }
    

    at least I would use less terse names:

    void _bswap16_field(void *field)
    {
         uint16_t value = *(uint16_t *)field;
         *field = _bswap16(value);
    }
    

    Don't have a C compiler handy but you might be able to make that void * a uint16_t * and still have it all work (saves you a cast).

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

What's with all the underscores? Names starting _ are very unpleasant to use in client code. This naming convention is usually associated with library internals. And even so, doesn't seem very necessary.


You could very well return directly instead of assigning the result to a variable:

return ((a & 0x00FF) << 8) | ((a & 0xFF00) >> 8);

That local scope inside _bswap_a is unusual and seems unnecessary:

{
    size_t array_size       = elem_size;
    size_t i                = 0;
    ....
}

Other functions that take pointers, like _bswap16f and _bswap32f, are not validating the inputs before dereferencing them. You should at least assert that the pointers are not null.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You are right, in client code underscores looks bad. I forgot you tell that I'm using this as a library internal. Therefore no checking in functions _bswap16f, _bswap32f. \$\endgroup\$ – marco-a Oct 5 '14 at 18:20
  • 1
    \$\begingroup\$ @d3l, even though the functions are only used internally, an assert check is still advisable to help the detection of errors during development. Remember that it will be disabled on production, so performance shouldn't be a concern. \$\endgroup\$ – glampert Oct 5 '14 at 19:15
2
\$\begingroup\$

See no value in making constants unsigned long long, but see future down-sides. If unsigned long long has a wider range than uint64_t, the following promotes the a & 0x00000000000000FFULL operation to one that is unsigned long long, an unnecessary expansion. OTOH an optimizing compiler may cope with that unnecessary widening. Suggest instead to only append the u suffix to insure constants are some unsigned type without making them extra wide.

uint64_t _bswap64(uint64_t a) {
  // a = ((a & 0x00000000000000FFULL) << 56) | 
  //    ((a & 0x000000000000FF00ULL) << 40) |  ...

  a = ((a & 0x00000000000000FFu) << 56) | 
      ((a & 0x000000000000FF00u) << 40) |   ...
      ...

Curious why suffixes were added in _bswap64(), but not _bswap32(). Suggest appending the u suffix.

uint32_t _bswap32(uint32_t a) {
  // a = ((a & 0x000000FF) << 24) |
  //    ((a & 0x0000FF00) <<  8) |  ...

  a = ((a & 0x000000FFu) << 24) |
      ((a & 0x0000FF00u) <<  8) |  ...
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.