current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

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);
}
share|improve this question
    
And the question is? –  Guffa 11 hours ago
    
I thought this is for reviewing code. I just wanted it to be reviewed. –  d3l 10 hours ago
    
Yes, but code can be reviewed for many aspects, for example readability, maintainability, portability, usefulness, complexity, performance, efficiency... You should specify why you think the the code should be reviewed, and for what. –  Guffa 10 hours ago

2 Answers 2

up vote 3 down vote

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.

share|improve this answer
    
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. –  d3l 9 hours ago
1  
@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. –  glampert 8 hours ago
up vote 1 down vote

  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).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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