Thread: Critique my code

I have something like this:

Code:

struct State{
 unsigned char *buf;

}

I want to read/write values from the buffer in chunks of 8, 16, and 32 bits.

This is the code I have come up with (which I would like someone to critique)

Code:

/* basically read the values from the last backwards */

static uint32_t read(State *s, size_t offset, size_t size)
{
    ssize_t i;
    uint32_t ret;

    if(!size){
        return 0;
    }

    ret = s->buf[offset + size - 1];

    for(i = size - 2; i >= 0; i--)
    {
        ret <<= 8;
        ret |= s->buf[offset + i];
    }

    return ret;
}

/* write the value from the first */

static void write(State *s, size_t offset, uint32_t val){
    size_t i;

    for (i = 0; i < size; i++)
    {
        s->buf[offset + i] = val & 0xff;
        val >>= 8;
    }
}

Ignoring the complication brought by the 'offset' part and assuming that 'i' indexes the array, is this the best way to do it ?

ssize_t is signed, so the loop will correctly not run if size=1.

On the code....
You seem to be missing a parameter for size in your write() function, but other than that it seems to work.

• You don't need to use ssize_t and size_t at all, since the maximum value of "size" is 4 bytes. If 'offset' points into an arbitrarily large buffer then I think size_t is the right thing to use there.
• Style comment: When assigning between uint32_t and char, it would be good to have explicit char casts to indicate exactly what's going on. They're not strictly necessary -- but then neither is the "& 0xff" you have to mask off the bottom byte during write() -- but it makes things clearer.
• Why do you have the assignment of the first byte outside of the loop? Might as well put it inside the loop and adjust the loop to do 1 more byte.

Looks fine to me -- byte-by-byte copies are the simplest way to deal with issues like alignment, so long as speed isn't a major issue.