Search code examples
ccompiler-warningsc99

standard way to solve "passing argument of function from incompatible pointer type" with function pointers parameter


I have a list module implementing a foward list, like the following (minimal working example):

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <stdbool.h>

struct list_cell {
    void* payload;
    struct list_cell* next;
};

struct list {
    struct list_cell* head;
    int length;
};

typedef bool(*matcher_t)(const void* data);

void* findElementInList(struct list* l, matcher_t matcher) {
    struct list_cell* tmp = l->head;
    while (tmp != NULL) {
        if (matcher(tmp->payload)) {
        return tmp->payload;
        }
        tmp = tmp->next;
    }
    return NULL;
} 

struct person {
    char* name;
    char* surname;
};


static char* constName = "Thomas";
bool matchByName(const struct person* p) {
    printf("comparing %s with %s\n", p->name, constName);
    return strcmp(p->name, constName) == 0;
}

int main() {
    //initializations (made by hand to have a MWE)
    struct person globalThomas = {"Thomas", "Lot"};

    struct list* l = (struct list*) malloc(sizeof(struct list));
    l->head = (struct list_cell*) malloc(sizeof(struct list_cell));
    l->head->payload = &globalThomas;
    l->head->next = NULL;
    l->length = 1;

    void* el = findElementInList(l, matchByName);
    if (el != NULL) {
    printf("found Thomas!\n");
    } else {
    printf("Thomas not found!\n");
    }
    //deallocation
    free(l->head);
    free(l);
}

Compiling this with gcc will generate the following warning:

list.c:57:34: warning: passing argument 2 of ‘findElementInList’ from incompatible pointer type [-Wincompatible-pointer-types]
  void* el = findElementInList(l, matchByName);
                                  ^
list.c:18:7: note: expected ‘matcher_t {aka _Bool (*)(const void *)}’ but argument is of type ‘_Bool (*)(const struct person *)’
 void* findElementInList(struct list* l, matcher_t matcher) {

This happens because matcher_t defined its parameter with void* but we inject struct person*. I want to solve this warning but I'm uncertain about the best way to solve it. Clearly (as proposed in in this SO answer) I can solve it by changing matchByName signature to matchByName(const void* p) and by casting void* pointer into struct person*. But I feel like this make the function the function purpose: by looking at the header only, I can't determine what is the input of the function. Leaving struct person* makes it instead clear.

So my question is: What the best method to solve this warning? What do you usually do to solve it? Is there another way to solve it aside changing matchByName signature?.

Thanks for any kind reply

PS: the objective here is to add -Werror flag in compilation in order to make the code more robust.


Solution

  • In C, this is the best you can do:

    bool matchByName(const void *px)
    {
        const struct person *p = px;
    
        printf("comparing %s with %s\n", p->name, constName);
        return strcmp(p->name, constName) == 0;
    }
    

    You have to make the function signature match its caller's expectation, and its caller is a pseudo-generic so it has to pass const void * rather than a concrete type. There is no way around this. But by assigning the untyped parameter to a variable with the correct concrete type as the first statement in the function, you make things clear for a human reading the code, and that's the best you can do.

    Incidentally, you really ought to get rid of that constName global variable, by having findElementInList accept an additional const void * parameter that it passes down to the matcher unmolested:

    bool matchByName(const void *px, const void *cx)
    {
        const struct person *p = px;
        const char *nameToMatch = cx;
    
        printf("comparing %s with %s\n", p->name, constName);
        return strcmp(p->name, constName) == 0;
    }
    
    void *findElementInList(struct list *l, matcher_t matcher,
                            const void *matcher_args)
    {
         struct list_cell *tmp = l->head;
         while (tmp) {
             if (matcher(tmp->payload, matcher_args)) {
                 return tmp->payload;
             }
             tmp = tmp->next;
         }
         return 0;
     }
    

    Please also note the stylistic corrections I have made to your code:

    • For historical reasons, in C preferred style is to put the opening curly brace of a function definition on its own line, even if all other opening curly braces are "cuddled" with their statement head. In some code you will also see the return type on its own line, which I think makes sense when there are often a lot of qualifiers on the return type, but don't do that unless you're going to do it consistently throughout the entire codebase.

    • The * in a pointer declaration binds to the thing on its right, not the thing on its left, so it should always be written with a space to its left and no space to its right. Everyone who says otherwise is wrong.

    • Explicit comparisons of pointers with NULL/0 are bad style; just write if (ptr) (or, in this case, while (ptr)). I personally think NULL itself is bad style and you should write 0 instead, but reasonable people can disagree about that.