0

I have this code which is supposed to swap two values inside a_ptr and b_ptr:

// swaps two values inside two variables of any type
void swap(void *a_ptr, void *b_ptr)
{
    size_t size = sizeof(void *);
    void *tmp = malloc(size);
    memcpy(tmp, a_ptr, size);
    memcpy(a_ptr, b_ptr, size);
    memcpy(b_ptr, tmp, size);
    free(tmp);

    // char wot0 = 0;
    // char wot1 = 0;
    // swap(&wot0, &wot1);
}

int main(){

    long a = -7;
    long b = 99999999;
    printf("A: %ld, B: %ld\n", a, b);
    swap(&a, &b);
    printf("A: %ld, B: %ld\n", a, b);
    printf("\n");

    short c = -9;
    short d = 11111;
    printf("C: %hd, D: %hd\n", c, d);
    swap(&c, &d);
    printf("C: %hd, D: %hd\n", c, d);
    printf("\n");

    char ca = 'a';
    char cx = 'x';
    printf("CA: %c  CX: %c\n", ca, cx);
    swap(&ca, &cx);
    printf("CA: %d  CX: %c\n", ca, cx);
    printf("\n");

    char *str0 = "Hello, ";
    char *str1 = "World!";
    printf("STR0: %s  STR1: %s\n", str0, str1);
    swap(&str0, &str1);
    printf("STR0: %s  STR1: %s\n", str0, str1);
    printf("\n");

    return 0;
}

However the output is:

A: -7, B: 99999999
A: 99999999, B: -7

C: -9, D: 11111
C: -7, D: -9

CA: a  CX: x
CA: -9  CX: a

STR0: Hello,   STR1: World!
STR0: World!  STR1: Hello, 

It successfully swaps a and b, and then somehow replaces c with b, and ca with d, how's that even possible?

Also, uncommenting these lines:

    // char wot0 = 0;
    // char wot1 = 0;
    // swap(&wot0, &wot1);

Leads to a segfault, why?

EDIT: I think I didin't convey my intentions very well. That I basically want to do is swap pointers so that a_ptr points to value inside b, and b_ptr points to value inside a, I don't want to actually copy the values themselves and I think I achieved that somewhat successfully, strings of different lengths (for examples "Foo" and "Hello, World!") get swapped without any issues, I tested that, however I don't understand why some variables don't get swapped and actually point to values outside of that I passed into the function

Omn
  • 39
  • 4
  • The targets to be swapped could be `char` or they could be `double` and the function c annot know what size to pass the `memcpy` unless explicitly told. – Weather Vane Dec 23 '21 at 23:28
  • Undefined behavior for accessing beyond the bounds of an object. – EOF Dec 23 '21 at 23:29
  • 2
    @WeatherVane What? `sizeof(void*)` is *perfectly* OK in principle, and not at all a compiler extension. – EOF Dec 23 '21 at 23:29
  • @EOF I didn't phrase it very well. I meant you cannot know the size of the target type. – Weather Vane Dec 23 '21 at 23:31
  • 1
    Do you want to change the values of the objects pointed to by `swap()`'s arguments? Then `sizeof( void * )` is not the correct number of bytes to copy, because you want to copy what the arguments *point to* -- `long`, `short`, `char` -- which you have no way of knowing. Or do you want to change the pointers (addresses) themselves? That does not work either, because pointers are passed *by value* themselves, and your `swap()` cannot make the caller's `a` to refer to `b` and vice versa. – DevSolar Dec 23 '21 at 23:32
  • 1
    @WeatherVane I would recommend deleting that first comment then, because it's still completely wrong after your edit. – EOF Dec 23 '21 at 23:32
  • @EOF partially, not completely. Edited. – Weather Vane Dec 23 '21 at 23:33
  • I think I didin't convey my intentions very well. That I basically want to do is swap pointers so that a_ptr points to value inside b, and b_ptr points to value inside a, I don't want to actually copy the values themselves and I think I achieved that somewhat successfully, strings of different lengths get swapped without any issues, I tested that, however I don't understand why some variables don't get swapped and actually point to values outside of that I passed into the function. – Omn Dec 24 '21 at 12:23

1 Answers1

3

sizeof(void *); is a constant (usually 4 or 8) and does not give you the size of the object it's pointing at. When you copy size bytes, you are not copying the correct amount for the types used.

You're probably better off by supplying the size of the type to the function:

// swaps two values inside two variables of any type
void swapper(void *a_ptr, void *b_ptr, size_t size)
{
    void *tmp = malloc(size);
    memcpy(tmp, a_ptr, size);
    memcpy(a_ptr, b_ptr, size);
    memcpy(b_ptr, tmp, size);
    free(tmp);
}

// Generate compilation error if objects of different sizes are used.
// The false switch case (0) can only be defined once so if the sizes
// are not the same, it'll try to redefine "case 0" and fail compiling:
#define compile_assert(_expr) switch (_expr) { case 0: break; case _expr: break; } 

#define swap(x,y) do { \
    compile_assert(sizeof(*(x)) == sizeof(*(y))); \
    swapper((x),(y),sizeof(*(x))); } while (0)

And call it like you aimed to do:

swap(&a, &b);

If you only need to swap fundamental types, you could make different implementations for all of them. This should also make it safer since it's harder to supply pointers to objects of different types this way:

#define SwapBuilder(name,type) \
    void name(type *a, type *b) { type tmp = *a; *a = *b; *b = tmp; }
SwapBuilder(swap_char,char)
SwapBuilder(swap_schar,signed char)
SwapBuilder(swap_uchar,unsigned char)
SwapBuilder(swap_short,short)
SwapBuilder(swap_ushort,unsigned short)
SwapBuilder(swap_int,int)
SwapBuilder(swap_uint,unsigned int)
SwapBuilder(swap_long,long)
SwapBuilder(swap_ulong,unsigned long)
SwapBuilder(swap_longlong,long long)
SwapBuilder(swap_ulonglong,unsigned long long)
SwapBuilder(swap_float,float)
SwapBuilder(swap_double,double)
SwapBuilder(swap_longdouble,long double)

// A _Generic to call the correct function:
#define swap(x,y) _Generic((x), \
    char* : swap_char, \
    signed char* : swap_schar, \
    unsigned char* : swap_uchar, \
    short* : swap_short, \
    unsigned short* : swap_ushort, \
    int* : swap_int, \
    unsigned int* : swap_uint, \
    long* : swap_long, \
    unsigned long* : swap_ulong, \
    long long* : swap_longlong, \
    unsigned long long* : swap_ulonglong, \
    float* : swap_float, \
    double* : swap_double, \
    long double* : swap_longdouble \
)((x),(y))

And you'd still call it with swap(&a, &b);

Ted Lyngmo
  • 93,841
  • 5
  • 60
  • 108
  • How does your proposed macro improve the situation here? – EOF Dec 23 '21 at 23:31
  • @EOF Sorry, I missed a part. Fixed – Ted Lyngmo Dec 23 '21 at 23:32
  • Can't say I like it, given that you can pass two things of different type/size in there, but I guess that's the joys of C rather than C++. – EOF Dec 23 '21 at 23:34
  • A fancier macro could also validate `sizeof *(x) == sizeof *(y)` and fail to compile when unequal, but let us leave that for another day. – chux - Reinstate Monica Dec 23 '21 at 23:43
  • 1
    @EOF and chux: Yeah, I know. It would be great with a type check before sending in the `void*`'s to the function. If support for only fundamental types are needed perhaps it could be made into a `_Generic` that calls type safe versions of the `swap` function. – Ted Lyngmo Dec 23 '21 at 23:44
  • one of the most common dupes – 0___________ Dec 23 '21 at 23:48
  • Hmmm, assuming objects do not overlap, could copy in chunks. Then a `malloc()/free()` not needed but a fixed size `buf[]`, say 32 (for `complex long double`). IAC an interesting, if common, exercise. – chux - Reinstate Monica Dec 23 '21 at 23:51
  • @chux-ReinstateMonica Yes, as long as one doesn't try to use it with objects bigger than the buffert size, that'd be a simpler solution. – Ted Lyngmo Dec 23 '21 at 23:55
  • 2
    I like your first solution the best. To add the "fancier" macro: `#define compile_assert(_expr) switch (_expr) { case 0: break; case _expr: break; }` and `#define swap(x,y) do { compile_assert(sizeof(*(x)) == sizeof(*(y))); swapper((x),(y),sizeof(*(x))); } while (0)` To me, this is size safe [if _not_ type safe] and _more_ generic (We could improve with `typeof`). Also, in `swapper`, for speed, we could use (1): `alloca` instead of `malloc/free` for _most_ usages. Or, (2): `static void *tmp; static size_t tmplen;` and do `if (size > tmplen) { tmp = realloc(tmp,size); tmplen = size; }` – Craig Estey Dec 24 '21 at 01:46
  • @CraigEstey Nice `compile_assert` - It gives a rather odd error message if one tries to use types of different sizes, but at least it gives an error. `alloca` would be nice too but since it's a non-standard function one would need to check if it's available first, or if it's available under a different name, like `_malloca` in Windows. – Ted Lyngmo Dec 24 '21 at 02:00
  • 1
    @CraigEstey `alloca()` alternative: VLA `unsigned char void tmp[size];` (standard - optionally , but no error check) – chux - Reinstate Monica Dec 24 '21 at 02:13
  • @chux-ReinstateMonica Hmm, yeah, that would be cool too. Too bad MS C compiler doesn't do VLA:s (yet). :-( – Ted Lyngmo Dec 24 '21 at 02:18
  • MS C skipped C99 and went from C89 to C11, in part just to skip the VLA requirement of C99. – chux - Reinstate Monica Dec 24 '21 at 02:20
  • @chux-ReinstateMonica lol, true story? :-) Well, I guess one could check `__STDC_NO_VLA__` and fallback to `alloca`, `_malloca` or a plain `malloc` as a last resort. – Ted Lyngmo Dec 24 '21 at 02:26
  • @TedLyngmo I think you are having too much fun! Nice macros. How about types `_Bool*, complex float*, ..., char **, void **` and use `default:` to go to `swapper()`? – chux - Reinstate Monica Dec 24 '21 at 06:22
  • 1
    @chux-ReinstateMonica X-mas mode. :-) True, the table is not as complete as I let on. I started looking at using `default:` to fallback to `swapper` yesterday but was to tired to get it done. Perhaps I'll return to this and update it later. – Ted Lyngmo Dec 24 '21 at 10:25
  • Using `typeof` in an if/assert doesn't work. But, it occurs to me that we can compare types for equality (and, by inference, get sizeof compares) by doing: `#define typeof_assert(_x,_y) if (0) *(_x) = *(_y) #define swap_typesafe(_x,_y) do { typeof_assert(_x,_y); swapper(_x,_y,sizeof(*(_x))); } while (0)` – Craig Estey Dec 24 '21 at 19:47
  • @CraigEstey I think implicit conversions will make that fail to fail a lot of cases, unless I'm misunderstanding it. I mean, `*(int*) = *(short*)` would pass, but `swapper` would use `sizeof(int)` for the `short` too. – Ted Lyngmo Dec 24 '21 at 21:27
  • 1
    You're right. So, if, in addition to the type match, we added back the assert for sizeof, we'd probably get closer. I was trying to keep the example brief, but I originally used both. – Craig Estey Dec 24 '21 at 22:22