Thread: Critique my code

  1. #1
    Tweaking master Aslaville's Avatar
    Join Date
    Sep 2012
    Location
    Rogueport
    Posts
    528

    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 ?

  2. #2
    Hurry Slowly vart's Avatar
    Join Date
    Oct 2006
    Location
    Rishon LeZion, Israel
    Posts
    6,793
    you declare size as size_t and i as well
    if user passed size = 1
    i will start with (size_t)-1 value which is positive...
    anyway size_t value is unsigned so condition i>=0 never gets false - you have and infinite loop which will crash due to out-of-bounds access
    All problems in computer science can be solved by another level of indirection,
    except for the problem of too many layers of indirection.
    David J. Wheeler

  3. #3
    Registered User
    Join Date
    Mar 2010
    Posts
    582
    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.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Please critique my source code - Craps Game
    By bos1234 in forum C Programming
    Replies: 8
    Last Post: 09-20-2013, 01:22 PM
  2. C code critique
    By mnd22 in forum C Programming
    Replies: 8
    Last Post: 12-23-2011, 10:05 AM
  3. Replies: 20
    Last Post: 12-09-2011, 03:28 PM
  4. Critique my code?
    By bigbadbowlindud in forum C++ Programming
    Replies: 20
    Last Post: 06-24-2004, 12:54 PM
  5. could ya'll critique this code
    By linuxdude in forum C Programming
    Replies: 0
    Last Post: 02-19-2004, 09:55 PM