Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I found out the **p can be removed, which leads to lesser codes, but I don't know whether it's better or worse since it introduces a warning, what do you think? #23

Open
Camio1945 opened this issue Oct 21, 2023 · 2 comments

Comments

@Camio1945
Copy link

with **p:

static inline list_item **find_indirect(list *l, list_item *target)
{
        list_item **p = &l->head;
        while (*p != target)
                p = &(*p)->next;
        return p;
}

without **p:

static inline list_item **find_indirect(list *l, list_item *target)
{
        while (l->head != target)
                l = &(l->head)->next;
        return l;
}

It works, but it introduces a warning:

Incompatible pointer types assigning to 'list *' (aka 'struct list *') from 'struct list_item **'

I'm not familiar with C, I don't know whether it's better or worse, what do you think?

@Iketurnher
Copy link

**p is essential in certain aspects

@ckwastra
Copy link

This is actually an interesting question. If we look at the definition of list:

struct list {
    list_item *head;
};

we could find that list is a struct with list_item * as its first (and only) member. This means list is nothing but a wrapper around list_item * and they could be used interchangeablely to some extent.

For instance, given p of type list_item ** and l of type list *, and they share the same value, one could conclude that *p is equivalent to l->head since both expressions return the pointed value (of type list_item *). This observation leads to the code in your question:

// Original version with best readability.
list_item **find_indirect_v1(list *l, list_item *target) {
    list_item **p = &l->head;
    while (*p != target) p = &(*p)->next;
    return p;
}

// `list` is just a wrapper around `list_item *`.
// Use explicit cast to suppress warnings.
list_item **find_indirect_v2(list *l, list_item *target) {
    while (l->head != target) l = (list *)&(l->head)->next;
    return (list_item **)l;
}

// What if `list' is really `list_item *`?
// Compare this with the original version.
typedef list_item *list_v3;
list_item **find_indirect_v3(list_v3 *l, list_item *target) {
    while (*l != target) l = &(*l)->next;
    return l;
}

Back to your question, is this implementation (find_indirect_v2 in the above code) better since we eliminated the use of a local variable (p in the example) compared to the original version? No (at least with modern compilers). All of the three implementations above produces same (optimal) assembly code with Clang in O1 optimization.

So general guideline is to perfer readability over some small and questionable optimizations like above as the compiler could do a better job in most of the time. (As a side note, the code in find_indirect_v2 is legal as long as list and head share same address, which is the case in our example.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants