Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

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.


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.


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.


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!


The standard actually specifies (or specified?) that the flexible array member must come after all the members, including the padding. But, from my reading, that wasn't their intent, and is certainly not how compilers implement it. The link in my original post is an acknowledgement from the committee about this and that the standard needs to be updated.

I think the main reason the OP got away with it is because in his structure, "sizeof(struct sdshdr)" is equal to the offset of "buf" in the struct. This is not necessarily true.

In particular, see the latest C standard draft (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf), section 6.7.2.1 (Structure and union specifiers), paragraph 18 and 20-21. An example in the standard uses this code:

    struct s { int n; double d[]; };
and says that "but it is possible that"

    sizeof (struct s) >= offsetof(struct s, d) + sizeof (double)


Yeah I was wrong on that, he must have sizes 8 both. My compiler gives 5 and 8.

Coincidentally I just read that specific paragraph earlier today for totally different reasons. :)

Will have to recheck some code.


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.


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.


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.


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


Exactly! For some reason this anti-pattern is very thoroughly entrenched among C programmers and it's so very frustrating.

My other favorite is casting the return value of malloc(), something you see a great deal of and that I always oppose. See http://stackoverflow.com/questions/605845/do-i-cast-the-resu... for my best arguments.


Casting the return value of malloc is necessary in C++. I think that's why it's so widespread, so you can compile your C program with a C++ compiler.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: