patterncMinor
Reverse array in C
Viewed 0 times
arrayreversestackoverflow
Problem
I wrote a function to reverse an array. The first argument is an iterable object, the second argument is the size of the array element in bytes, and the third argument is the length of the array.
Could this implementation cause errors? How can I improve it?
void reverse(void *object, int size, int length) {
int i;
int j;
int k;
for(i=0, j=length-1; i < j; i++, j--) {
for(k=0; k<size; k++){
*((char*)object+(i * size)+k) = *((char*)object+(i * size)+k) ^ *((char*)object+(j * size)+k);
*((char*)object+(j * size)+k) = *((char*)object+(i * size)+k) ^ *((char*)object+(j * size)+k);
*((char*)object+(i * size)+k) = *((char*)object+(j * size)+k) ^ *((char*)object+(i * size)+k);
}
}
}Could this implementation cause errors? How can I improve it?
Solution
Your code works – as far as I can see – correctly. But I would rewrite
it a bit to improve the readability (and with it the maintainability) of the
code.
The function name
to reversing an array, string, or whatever. A better name might be
The repeated cast
a variable
once. Next, the address calculations can be simplified by separating
the swapping of two objects into a helper function:
Note also how
Using your method,
However, I can see no advantage in this "magically swap two bytes without
temporary storage" and would rewrite it as
which is much simpler (no XOR operations and less memory reads and
writes). It is possible to make the swap operation faster by using
check of the sizes and memory alignments.
Here you can get rid of the subcripts by increasing the
passed pointers
but that is a matter of taste. I don't think that it makes a difference
if you compile the code with optimizations switched on.
In addition, I would use
parameters.
of an object in memory, see for example the declarations
of
it a bit to improve the readability (and with it the maintainability) of the
code.
The function name
reverse() is quite general. That could referto reversing an array, string, or whatever. A better name might be
reverseArray(). The repeated cast
(char*)object can be avoided by defininga variable
char *ptr = objects;once. Next, the address calculations can be simplified by separating
the swapping of two objects into a helper function:
void reverseArray(void *objects, int size, int length) {
char *ptr = objects;
for (int i = 0, j = length - 1; i < j; i++, j--) {
swapObjects(ptr + size * i, ptr + size * j, size);
}
}Note also how
i and j are declared locally to the for-loop.Using your method,
swapObjects() would be:static inline void swapObjects(char *o1, char *o2, int size) {
for (int k = 0; k < size; k++) {
o1[k] = o1[k] ^ o2[k];
o2[k] = o1[k] ^ o2[k];
o1[k] = o1[k] ^ o2[k];
}
}However, I can see no advantage in this "magically swap two bytes without
temporary storage" and would rewrite it as
static inline void swapObjects(char *o1, char *o2, int size) {
for (int k = 0; k < size; k++) {
char tmp = o1[k];
o1[k] = o2[k];
o2[k] = tmp;
}
}which is much simpler (no XOR operations and less memory reads and
writes). It is possible to make the swap operation faster by using
int or long as temporary storage, but this requires a carefulcheck of the sizes and memory alignments.
Here you can get rid of the subcripts by increasing the
passed pointers
static inline void swapObjects(char *o1, char *o2, int size) {
for (int k = 0; k < size; k++, o1++, o2++) {
char tmp = *o1;
*o1 = *o2;
*o2 = tmp;
}
}but that is a matter of taste. I don't think that it makes a difference
if you compile the code with optimizations switched on.
In addition, I would use
size_t as type for the size and lengthparameters.
size_t is what sizeof returns, and is the correct type to describe the sizeof an object in memory, see for example the declarations
of
malloc(), calloc() or strlen().Code Snippets
char *ptr = objects;void reverseArray(void *objects, int size, int length) {
char *ptr = objects;
for (int i = 0, j = length - 1; i < j; i++, j--) {
swapObjects(ptr + size * i, ptr + size * j, size);
}
}static inline void swapObjects(char *o1, char *o2, int size) {
for (int k = 0; k < size; k++) {
o1[k] = o1[k] ^ o2[k];
o2[k] = o1[k] ^ o2[k];
o1[k] = o1[k] ^ o2[k];
}
}static inline void swapObjects(char *o1, char *o2, int size) {
for (int k = 0; k < size; k++) {
char tmp = o1[k];
o1[k] = o2[k];
o2[k] = tmp;
}
}static inline void swapObjects(char *o1, char *o2, int size) {
for (int k = 0; k < size; k++, o1++, o2++) {
char tmp = *o1;
*o1 = *o2;
*o2 = tmp;
}
}Context
StackExchange Code Review Q#115625, answer score: 5
Revisions (0)
No revisions yet.