16

I am writing a function that receives a pointer to a comparison function and an array of MyStructs and is supposed to sort the array according to the comparison function:

void myStructSort(
                  struct MyStruct *arr,
                  int size,
                  int (*comp)(const struct MyStruct *, const struct MyStruct *)) {
  qsort(arr, size, sizeof(struct MyStruct), comp);
}

Unfortunately this doesn't compile because qsort expects the comparator to receive void * arguments and not const struct MyStruct *. I thought of several bad solutions and was wondering what the correct solution is.

Option 1

Cast comp to int (*)(const void *, const void*). This compiles but is undefined behavior (see this SO question).

Option 2

Create a global variable int (*global_comp)(const struct MyStruct *, const struct MyStruct *) and set global_comp=comp inside myStructSort. Then create a function:

int delegatingComp(const void *a, const void *b) {
  return globalComp((const struct MyStruct *)a, (const struct MyStruct *)b);
}

And in myStructSort call qsort(arr, size, sizeof(struct MyStruct), delegatingComp). The problem with this is the icky global variable.

Option 3

Reimplement qsort. This is functionally safe but very bad practice.

Is there a magical perfect fourth option?

Edit

I can't change the API of myStructSort and I am compiling my code using gcc c99 -Wall -Wextra -Wvla.

Community
  • 1
  • 1
Benjy Kessler
  • 7,356
  • 6
  • 41
  • 69
  • In that case, a wrapper function like the one you came up with in option 2 is the best approach. Btw, tbh I didn't quite get the idea about global variables you mentioned. What are they for? – HighPredator Aug 11 '15 at 13:22
  • HighPredator@, `delegatingComp` needs to know which function to call and it can't be passed to it as an argument because it needs to match the argument of `qsort`. – Benjy Kessler Aug 11 '15 at 13:27
  • if you are using `gcc`, gnu extension allows you to define a subfunction inside of a function. it simulates a stack based closure. if you don't mind portability damage, you can try that. – Jason Hu Aug 11 '15 at 13:27
  • @BenjyKessler, ahh, now I get it. – HighPredator Aug 11 '15 at 13:29
  • qsort API is basically broken, there is no good solution for this problem. – n. m. could be an AI Aug 11 '15 at 13:38
  • @BenjyKessler looks like standard C99 is explicitly selected. You're out of luck. – Quentin Aug 11 '15 at 13:39
  • @n.m. how broken is that? could you explain? – Jason Hu Aug 11 '15 at 13:39
  • Is there a problem with converting the `const void *` parameters to whichever other pointer type is required? It's one line of code per comparator... I could think of faaaar more significant targets for optimisation. – autistic Aug 11 '15 at 13:40
  • @HuStmpHrrr it won't work. Casting the function pointer is WAY more portable anyway. – n. m. could be an AI Aug 11 '15 at 13:40
  • @n.m. what do you mean `qsort` api is broken. i don't think casting will help if the api is broken. – Jason Hu Aug 11 '15 at 13:41
  • @n.m. Casting the function pointer isn't portable, because `qsort` will end up calling the function as though it recieves `const void *` representation when that pointer representation may differ from the actual representation. – autistic Aug 11 '15 at 13:41
  • @n.m. I doubt undefined behaviour is all that portable. Sure, the random effects you get out of it may be the same, but I wouldn't call that "portable". – Quentin Aug 11 '15 at 13:42
  • @Quentin This question is about optimising keystrokes, I gather, right? – autistic Aug 11 '15 at 13:44
  • @BenjyKessler Why do you think a global is necessary? Care to explain? – autistic Aug 11 '15 at 13:44
  • Well technically not global but static. But it can't be passed to `delegatingComp` as an argument. – Benjy Kessler Aug 11 '15 at 13:45
  • @BenjyKessler Just one note which I'm not happy to convert to an answer, because I think this design is silly and don't approve of it, but suppose in your option 2 you use the first argument as a boolean; if it's non-null you perform a comparison (by calling the function) and if it's null you set the `static` variable to whatever is in the second argument (you can use `struct` or `union` to pass a function pointer)... This would allow you to move the *global* into the function (as a `static` variable), but I'm sure that's not what you want, and it's still bloody hideous! – autistic Aug 11 '15 at 14:55
  • Yeah, I thought of something like that but I figured that the code complexity will be higher in this implementation than just rewriting qsort so I discarded it. – Benjy Kessler Aug 11 '15 at 15:23

5 Answers5

10

Option 2 breaks thread-safety, so I wouldn't choose that one.

Option 3 is just plain wrong as you point out. There is no reason to re-implement quicksort and potentially make a mistake.

Option 1 is UB but it will work on any sane compiler. If you choose this option be sure to add a comment.

I would also consider:

Option 4. Redesign the interface of myStructSort to take int (*)(const void *, const void*) or scrap it entirely and call qsort directly. Basically send it back to the architecht, because he made a poor design choice.

Klas Lindbäck
  • 33,105
  • 5
  • 57
  • 82
  • 4
    For option 2, you can put the 'global' in thread-local storage. – Colonel Thirty Two Aug 11 '15 at 19:29
  • Nice overview of the available options! For me, this makes it pretty clear that the only actually sane option is Option 1, i.e. simply cast the function pointer. Everything else is a cure worse than the disease. – fgp Aug 11 '15 at 20:27
5

following approach only works for gcc. It's a part of gnu extension. further please reference to https://gcc.gnu.org/onlinedocs/gcc-4.8.5/gcc/Nested-Functions.html#Nested-Functions

first let's make sure the prototype of qsort is in such a form:

void qsort(void *base, size_t nmemb, size_t size,
           int (*compar)(const void *, const void *));

then you can:

void myStructSort(
                  struct MyStruct *arr,
                  int size,
                  int (*comp)(const struct MyStruct *, const struct MyStruct *)) {
  int comparator(const void * a, const void *b) {
    return comp((const struct MyStruct *)a, (const struct MyStruct *)b);
  }
  qsort(arr, size, sizeof *arr, comparator);
}

But again, since it uses gnu extension, don't expect too much portability.

ABOUT YOUR COMMENT: for modern gcc, gnu standard is default instead of iso ones. specifically, lastest gcc should use gnu11 standard. older ones are using gnu89. so, I don't know about your command line params, but if -std is not set, this will work.

following is an example taken from info gcc, just in case the link is dead. it shows a closure-like usage of nested function:

 bar (int *array, int offset, int size)
 {
   int access (int *array, int index)
     { return array[index + offset]; }
   int i;
   /* ... */
   for (i = 0; i < size; i++)
     /* ... */ access (array, i) /* ... */
 }
Jason Hu
  • 6,239
  • 1
  • 20
  • 41
4

If you are using gcc, then you can use the qsort_r function in glibc since 2.8, which allows you to specify a comparator function with an additional user-supplied argument:

void qsort_r(void *base, size_t nmemb, size_t size,
             int (*compar)(const void *, const void *, void *),
             void *arg);

This is not portable, of course, and it requires you to define the feature-test macro:

#define _GNU_SOURCE

(On FreeBSD -- and, presumably, Mac OS X -- there is a similar but incompatible qsort_r; the difference is that the user-supplied context argument is provided as the first argument to the comparison function, rather than the last argument.)

But if you have it, it allows you to avoid the global in option 2:

/* This struct avoids the issue of casting a function pointer to
 * a void*, which is not guaranteed to work. It might not be
 * necessary, but I know of no guarantees.
 */
typedef struct CompContainer {
   int (*comp_func)(const struct MyStruct *, const struct MyStruct *);
} CompContainer;

int delegatingComp(const void *a, const void *b, void* comp) {
  return ((CompContainer*)comp)->comp_func((const struct MyStruct *)a,
                                           (const struct MyStruct *)b);
}

void myStructSort(
              struct MyStruct *arr,
              int size,
              int (*comp_func)(const struct MyStruct *,
                               const struct MyStruct *)) {
  const CompContainer comp = {comp_func};
  qsort_r(arr, size, sizeof(struct MyStruct), delegatingComp, &comp);
}

(Live on ideone)

rici
  • 234,347
  • 28
  • 237
  • 341
  • I think you could do without the wrapper struct. A simple pointer-to-pointer-to-function would be good enough. It's a data pointer so it will survive a round trip through `void *` –  Aug 11 '15 at 16:47
  • @WumpusQ.Wumbley: That's not guaranteed by the C standard, and I don't know if gcc guarantees it either. – rici Aug 11 '15 at 16:52
  • @rici Both POSIX and Windows (which exhausts the set of ABIs you are liable to encounter in practice) do in fact guarantee that pointers to functions can be cast to `void *` and back without loss of information, in order not to have to have two variants of `dlsym` / `GetProcAddress` for object and function symbols. Also, I believe Wumpus Q. Wumbley is correct: a pointer *to* a pointer to a function is a data pointer and therefore round-trippable through `void *` in standard C. – zwol Aug 11 '15 at 19:45
  • You are correct about @wumpus; I misread the comment and I apologize for that. The struct adds no overhead, though. An actual pointer to the posix requirement would be helpful, but I can search it out when I get home. – rici Aug 11 '15 at 23:37
  • @rici POSIX: http://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html : last paragraph of the APPLICATION USAGE section. – zwol Aug 12 '15 at 17:12
  • @rici Windows: https://msdn.microsoft.com/en-us/library/windows/desktop/ms683212%28v=vs.85%29.aspx does not state this requirement explicitly, but it is implied by `GetProcAddress` being able to retrieve the address of a "function _or_ variable". (Note that `FARPROC` is, iirc, a function pointer type.) – zwol Aug 12 '15 at 17:15
  • @zwol: That's a pretty well hidden requirement :) Thanks. – rici Aug 12 '15 at 18:34
3

The correct approach is to cast from void const * to MyStruct const * in the comparison function.

This is well-defined for the first object, because the pointer that was passed to the comparison function was created by a cast from MyStruct const * to void const *, and casting a pointer to void back to its original type is allowed (and it's really the only thing that is).

For the other array members, it is assumed that casting void const * to char const *, adding the offset of the object, generated by multiplying the object size with the position of the object in the array, and casting that back to void const * will give a pointer that can be cast back to MyStruct const *.

That is a bold assumption, but usually works out. There may be corner cases where this doesn't work, but in general compilers pad any struct foo to a multiple of its alignment to ensure that array members' start addresses have a distance of sizeof(struct foo).

Casting function pointers is generally unsafe and needs to be avoided, as different data types may have different representations -- for example, a void * must be able to express every possible address as it could have been converted from a char *, while a MyStruct * is guaranteed to have a few of the least significant bits clear as any valid object would be aligned -- so it is entirely possible that the calling convention for these types could be different.

Simon Richter
  • 28,572
  • 1
  • 42
  • 64
1

The only sane option is to re-write the interface you've created, or make a new one.

I've done something very similar with bubble sort on another answer of mine.

In short, with C, you want your sort function to be of the form:

void* bubbleSort(void* arr, int (*compareFcn)(void*, void*),
    size_t sizeOfElement, size_t numElements)

And your comparison function to be of the form:

int compareFunction(void *a, void *b);
Community
  • 1
  • 1
Cloud
  • 18,753
  • 15
  • 79
  • 153