Skip to main content
Code Review

Return to Revisions

3 of 6
added 168 characters in body
Martin R
  • 24.2k
  • 2
  • 38
  • 96

Your code works – as far as I can see – correctly now. But I would rewrite it to improve the readability (and with it the maintainability) of the code.

First, the repeated cast (char*)object can be avoided by defining a variable

char *ptr = objects;

once. Next, the address calculations can be simplified by separating the swapping of two objects into a helper function:

void reverse(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);
 }
}

where swapObjects() is:

static inline void swapObjects(char *o1, char *o2, int size) {
 for (int c = 0; c < size; c++, o1++, o2++) {
 *o1 = *o1 ^ *o2;
 *o2 = *o1 ^ *o2;
 *o1 = *o1 ^ *o2;
 }
}

However, I can see no advantage in this "magically swap two bytes without temporary storage" and would rewrite this simply as

static inline void swapObjects(char *o1, char *o2, int size) {
 for (int c = 0; c < size; c++, o1++, o2++) {
 char tmp = *o1;
 *o1 = *o2;
 *o2 = 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 careful check of the sizes and memory alignments.)

In addition, I would use size_t as type for the size and length parameters. size_t is the correct type to describe the size of an object in memory, see for example the declarations of malloc() and calloc().

Martin R
  • 24.2k
  • 2
  • 38
  • 96
default

AltStyle によって変換されたページ (->オリジナル) /