Is it safe to swap values of an unknown type

Question

Let's say we have the following structure:

typedef struct array
{
    size_t  sz_of  // sizeof an object;
    size_t  count;
    void*   objects;
}Array;

Well, an array which can contain a set of arbitrary types. The following functions are designed to be applied on this structure:

Array* ArrayNew(size_t len, size_t sz_of){
    Array* a = malloc(sizeof(Array));
    if(a == NULL)
        return NULL;

    a->sz_of = sz_of;
    a->count = len;
    a->objects = malloc(len * sz_of);
    if(a->objects == NULL){
        free(a);
        return NULL;
    }

    return a;
}

void ArrayDestroy(Array* a){
    free(a->objects);
    free(a);
}

void* ArrayGet(Array* a, size_t i){
    if(i > a->count)
        return NULL;

    return (a->objects + a->sz_of * i);
}

void ArraySet(Array* a, void* v, size_t i){
    if(i > a->count)
        return;

    memcpy(a->objects + a->sz_of * i, v, a->sz_of);
}

We have a function:

which act as a constructor;
which act as a destructor;
which act as a getter;
which act as a setter.

I have added a function which swap two elements of the array but I am not sure if I do it correctly. Since a char is 1 byte long, I am temporarily storing an element in a char array which is sz_of long:

void ArraySwap(Array* a, size_t x, size_t y){
    if(x < a->count && y < a->count){
        char t[a->sz_of];
        memcpy(t, a->objects + a->sz_of * x, a->sz_of);
        memcpy(a->objects + a->sz_of * x, a->objects + a->sz_of * y, a->sz_of);
        memcpy(a->objects + a->sz_of * y, t, a->sz_of);
    }
}

x and y are the indexes of the objects which have to be swapped.

It works well on my configuration, but is it unsafe or a non-standard way to proceed? More globally, if you see something weird, unsafe or non-standard, could you point me out please?

Thanks for your answers.

To my mind, the only thing you should worry about is allocating enough space for the `objects`. — ForceBru, Oct 17 '17 at 19:47
@ForceBru This is done by multiplying len which represents the number of objects and sz_of which is the size in bytes of an objects (in ArrayNew). — Papipone, Oct 17 '17 at 19:52
Rather than using a variable-length array, I would suggest using an array of a fixed moderate size (e.g. 256 bytes) and then performing a swap of more by 256 bytes by swapping 256 bytes, advancing both pointers by 256 bytes, and swapping the remainder. I'd also suggest testing for the case where source and destination are equal and skipping the operation if they are. Even if cases where they're equal are rare enough that it would be faster to blindly do the three memcpy operations, an attempt to memcpy when source and destination are equal may result in... — supercat, Oct 17 '17 at 19:59
...compilers optimizing *other* code on a presumption that they won't be. — supercat, Oct 17 '17 at 19:59
OT: Consider `a->objects = malloc(len * sz_of);` --> `a->objects = calloc(len, sz_of);` to give the elements a starting value or "zero". Better yet, post on https://codereview.stackexchange.com for additional ideas on working code. — chux - Reinstate Monica, Oct 17 '17 at 20:59
@supercat: There's code in musl libc qsort implementing exactly that approach for swapping (actually more general cyclic permutation) elements of the array being sorted. — R.. GitHub STOP HELPING ICE, Oct 17 '17 at 23:32

chux - Reinstate Monica · Accepted Answer · 2017-10-17T20:35:29.450

Code is relying on char* like pointer math with void*. Some compilers treat void*pointer math like char*, but best not to assume that non-standard behavior.

a->objects + a->sz_of * x // not potable.

Instead use unsigned char * or char * and consider a fixed size buffer @supercat. This avoids use of a variable length array (VLA), something that is unbounded here and only optionally support in C11.

void ArraySwap(Array* a, size_t x, size_t y){
  if(x < a->count && y < a->count){
    unsigned char *xp = a->objects;
    xp += x*a->sz_of;
    unsigned char *yp = a->objects;
    yp += y*a->sz_of;
    unsigned char buf[64];
    for (size_t i=0; i<a->sz_of; i += sizeof buf) {
      size_t sz = a->sz_of - i;
      if (sz > sizeof buf) {
        sz = sizeof buf;
      }
      memcpy(t, xp, sz);
      memcpy(xp, yp, sz);
      memcpy(yp, t, sz);
      xp += sz;
      yp += sz;
    }
  }
}

score 2 · Answer 2 · edited Jun 20 '20 at 09:12

You don't have to worry about the size of the temporary array. In C99, it is always true that sizeof(char) == 1.

That means that the t array on the stack size will be a->sz_of bytes.

There is another problem that might occur. When allocating t on the stack, you use a C99 feature called VLA (variable-length array). This might be problematic in two cases:

You want to support C89 compilers (which I assume you don't).
Your objects size might be so big, there won't be enough space on the stack, leading to undefined behaviour. If you worry about such case, and want to handle it correctly, it will be better to allocate memory on the heap (using malloc), because this way you can check for errors in memory allocation (comparing the result to NULL).

Lastly, that is probably not a problem, but it seems better to me to use the ArrayGet function in your implementation of ArraySwap, instead of calculating the elements` address manually.

Is it safe to swap values of an unknown type

2 Answers2