6/17/2010

06-17-10 - Friday Code Review

Today I bring you a piece of my own code that had a nasty bug. This is from my buffered file reader, which I did some fiddling on a while ago and didn't test hard enough (there are now so many ways to use my code that it's really hard to make sure I test all the paths, which is a rant for another day).

Here's the whole routine :


   S64  OodleFile_ReadLarge(OodleFile * oof,void * into,S64 count)
   {
    // use up the buffer I have :
    U8 * into_ptr = (U8 *)into;
    S64 avail = (S64)( oof->ptrend - oof->ptr );
    S64 useBuf = RR_MIN(avail,count);
        
    memcpy(into_ptr,oof->ptr,(size_t)useBuf);
    oof->ptr += useBuf;
    into_ptr += useBuf;
    count -= useBuf;
    
    rrassert( oof->ptr <= oof->ptrend );
        
    if ( count > 0 )
    {
        //OodleFile_Flush(oof);
        rrassert( oof->ptr == oof->ptrend );
        
        S64 totRead = 0;
        
        while( count > 0 )
        {
            S32 curRead = (S32) RR_MIN(count,OODFILE_MAX_IO_SIZE);
            
            S32 read = oof->vfile->Read(into_ptr,curRead);
        
            if ( read == 0 )
                break; // fail !
        
            count    -= read;
            into_ptr += read;
        }
        
        return useBuf + totRead;
    }
    else
    {
        return useBuf;
    }
   }

But the problem is here :


        S64 totRead = 0;
        
        while( count > 0 )
        {
            S32 curRead = (S32) RR_MIN(count,OODFILE_MAX_IO_SIZE);
            
            S32 read = oof->vfile->Read(into_ptr,curRead);
                
            count    -= read;
            into_ptr += read;
            // totRead += read;  // doh !!
        }
        
        return useBuf + totRead;

That's benign enough, but I think this is actually a nice simple example of a habit I am trying to get into :

never use another variable when one of the ones you have will do

In particular this loop could be written without the redundant counter :


        while( count > 0 )
        {
            S32 curRead = (S32) RR_MIN(count,OODFILE_MAX_IO_SIZE);
            
            S32 read = oof->vfile->Read(into_ptr,curRead);
                
            count    -= read;
            into_ptr += read;
        }
        
        return useBuf + (S64)(into_ptr - (U8 *)into);

Don't step the pointer and also count how far it steps, do one or the other, and compute them from each other. I keep finding this over and over and its one of the few ways that I still consistently make bugs : if you have multiple variables that need to be in sync, it's inevitable that that will get screwed up over time and you will have bugs.

(BTW I just can't help going off topic of my own posts; oh well, here we go : I hate the fact that I have to have C-style casts in my code all over now, largely because of fucking 64 bit. Yeah yeah I could static_cast or whatever fucking thing but that is no better and much uglier to read. On the line with the _MIN , I know that a _MIN of two types can fit in the size of the smaller type, so it's safe, but it's disgusting that I'm using my own semantic knowledge to validate the cast, that's very dangerous. I could use my check_value_cast here but it is tedious sticking that everywhere you do any 32-64 bit casting or pointer-int casting. On the output I could use ptrdiff_t or whatever you portability tards want, but that only pushes the cast to somewhere else (if I Read return a type ptrdiff_t then the outside caller has to turn that into S64 or whatever). I decided it was better if my file IO routines always just work with 64 bit file positions and sizes, even on 32 bit builds. Obviously running on 32 bits you can't actually do a larger than 4B byte read into a single buffer, so I could use size_t or something for the size of the read here, but I hate types that change based on the build, I much prefer to just say "file pos & size is 64 bit always".)

No comments:

old rants