Comment by unwind

12 years ago

Nice, thanks for contributing (even more).

I haven't read much of the code at all, but a minor suggestion would be to change:

    struct sdshdr *sh = (void*)(s-(sizeof(struct sdshdr)));

to

    struct sdshdr *sh = (void*)(s-sizeof *sh);

since the entire idea is that the pointer on the left-hand side is to the type whose size should be subtracted, I think it's better not to repeat the type but to "lock it" to the pointer instead. This also (of course) means we can drop the parenthesis with sizeof, since those are only needed when its argument is a type name.

11 comments

unwind

Reply
antirez  12 years ago

Thanks unwind, this is much better indeed! Fixing.

EDIT: Fixed here: https://github.com/antirez/sds/commit/c636fc6cd25e455a75dca2...

Seems everything is still working but I need definitely more unit tests... Note everything is covered right now.

  • gandalf013  12 years ago

    A more correct version might be:

        #include <stddef.h>

    int buf_offset = (int)offsetof(struct sdshdr, buf);
    struct sdshdr *sh = s - buf_offset;

    This is because the compiler might insert padding between your struct elements and the flexible array member. In your case, you're using only int types in the header, so padding shouldn't be an issue on most architectures, but consider the following:

        struct header {
        int i;
        char c;
        char data[];
    };

    sizeof(struct header) on my machine is 8, but "data" starts at an offset of 5 from the beginning of the struct. So, to go from "data" pointer to the pointer representing the beginning of the struct, you will need to subtract 5, not 8. Here is a test program:

        #include <stdio.h>
    #include <stddef.h>

    struct header {
        int i;
        char c;
        char data[];
    };

    int main(void)
    {
        struct header h = {0};
        printf("offsetof %zu\n", offsetof(struct header, data));
        printf("sizeof %zu\n", sizeof h);
        printf("start %p, data start %p, delta %d\n",
                (void *)&h, (void *)(h.data), (int)((char *)h.data - (char *)&h));

        return 0;
    }

    http://std.dkuug.dk/JTC1/SC22/WG14/www/docs/n983.htm is relevant for this case as well.

    • deletes  12 years ago

      Thank you for that.

      That surprised me. I thought( and read somewhere ) that the flexible array member comes right after the entire struct, which includes possible padding.

      OP got away with it since he always allocates the size of the entire struct, which is 8, plus the string size. And since char doesn't have any alignment requirements it doesn't matter, because he always gets back to correct offset. So data member is basically not used, and if you check the code you will see that it actually in never used!

      2 replies →

    • unwind  12 years ago

      Very true of course, I guess I assumed that antirez had thought of that, should have noticed that there were no packing enhchantmens on the struct. I love offsetof() for code like this.

simias  12 years ago

Heh, you're the first person I meet who advocates dropping the parens around the sizeof argument in any situation. That does look terribly alien to me.

  • breadbox  12 years ago

    Really? I always do. If you don't, it makes the variable look like a type. It's the same reason why I dislike extra parens around the value in the return statement. Don't make it look like something it isn't.

  • adestefan  12 years ago

    sizeof is a unary operator and not a function. Only types need to be enclosed in parens. Most people don't know this.