Help with c++ logic?

Something::methodname()
{  
    (unsigned char*) ptr = (unsigned char*) m_pptr;

    while ((*ptr || *(ptr+1)) && (((unsigned char*)m_pptr+BUFSIZE)<ptr))
        ptr++;

    if(ptr == m_pptr)
        return ptr; 

    return ptr + 1;
}

m_pptr is a protected me开发者_StackOverflow中文版mber of a class. ptr is local to this function

Could someone help me with the logic of this code? I know it compiles but the answers I'm getting out are not the ones I'm expecting. I am memset-ing a buffer full of A5's and the while loop fails somehow. It skips right past it. Any help would be great.

This will go through a buffer and if the value of the pointer or the value of (ptr+1) is true it will increment the pointer AND the ptr can't exceed the size of the buffer(which is found by m_pptr "pointer to the beginning of the buffer" + buffer size) has to be true also. The if statement says if m_pptr(pointer to beginning of the buffer is the same as ptr then return just the pointer.

this function returns a void* and is passed nothing

 (((unsigned char*)m_pptr+BUFSIZE)<ptr))

looks backward:

 (((unsigned char*)m_pptr+BUFSIZE)>ptr))

would be more likely; Even more sane:

while (ptr < ((unsigned char*) m_pptr + BUFSIZE)) // until end of buffer
{
    if (!*ptr)      // null char reached
        break;
    if (!*(ptr+1))  // null char almost reached
        break;

    // do stuff

    ptr++;
}

This bit looks suspicious to me:

 while ((*ptr || *(ptr+1))

Imagine that ptr is pointing to a valid character byte, followed by a NUL terminator byte.

The first sub-test of the above line will evaluate to true, and so ptr gets incremented. Now ptr is pointing at the NUL terminator byte, and *(ptr+1) is pointing at the byte AFTER the NUL terminator byte... which might be garbage/undefined, and therefore might be non-zero, at which point (ptr) will be incremented again (because the second sub-test evaluated to true this time), so that ptr now points to the byte AFTER the NUL terminator byte. And from there on your pointer heads off into la-la-land, trying to interpret data that was never meant to be part of the string it was parsing.

Wouldn't it look cleaner and simpler if you used for-loop instead?

for ( int i =0; i<BUFSIZE && (ptr[i] || ptr[i+1]); i++);

It would be easier to notice wrong comparison, wouldn't it? And i think it would be also easier to see that in this case it should be

for ( int i =0; i<(BUFSIZE-1) && (ptr[i] || ptr[i+1]); i++);

or even

for ( int i =1; i<BUFSIZE && (ptr[i-1] || ptr[i]); i++);

unless obiviously you accounted for that by having BUFSIZE equal to buffer size minus one.