Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I received this question at a C++ programming interview:

void memcpy(char* dst, const char* src, int numBytes)
{ 
    int* wordDist = (int*)dst;
    int* wordSrc = (int*)src;
    int numWords = numBytes >> 2;  
    for (int i = 0; i < numWords; i++)
    {
        *wordDist++ = *wordSrc++;  
    }
    int whatisleft = numBytes >> 2 - (numWords );
    dst = (char*)wordDist;
    src = (char*)wordSrc;
    for (int i = 0 ; i <= whatisleft; i++);
    {
        *dst++ = *src++;
    }
}
  1. What are portability issues?
  2. At least 1 syntax error?
  3. Algorithm considerations?

Now I found that this line:

int whatisleft = numBytes >> 2 - (numWords );

is wrong as it doesn't calculates the correct number of bytes left to copy.

The correct one should be:

int numBytesInWord = (numWords << 2 );
int whatisleft = numBytes  - numBytesInWord;

At the end, the copied char array is several characters shorter than the source.

share|improve this question
    
Use % instead of shift/subtract to get whatisleft. Shifts are best for logical operations not sums. The compiler may of course choose to use a shift at the machine level. –  William Morris Dec 12 '13 at 17:40
1  
CR isn't about "find the errors in this piece of code" - please see our Help Center –  Mat's Mug Dec 12 '13 at 18:07
1  
Teh biggest problem is 2. Who said the size of integer is 2 bytes? You should be using sizeof(int) –  Loki Astari Dec 14 '13 at 18:22
    
@LokiAstari that's 4... right shift by 2 is dividing by 4 or the most common (but far from only possible) size of int. –  Emily L. Jul 8 at 19:49
1  
You're not using x86 or x86-64 at all? They all have sizeof(int) == 4... –  Emily L. Jul 8 at 20:07

4 Answers 4

up vote 7 down vote accepted

There's a number of issues with this code:

int* wordDist = (int*)dst;
int* wordSrc = (int*)src;

Even if this works on a given platform, it is definitely not portable. int* can often have more strict alignment requirements than char* - this is technically undefined behaviour.

Further, (int*)src; is casting away constness - always dangerous if you then modify the value somewhere in the rest of the function.

int numWords = numBytes >> 2; 

This assumes that an int is 32 bit, which is again non-portable, and will of course be wrong on any platform where this isn't the case. This could be fixed by using:

int numWords = numBytes / sizeof(int);

However, the alignment issues mentioned previously still remain.

This code:

int whatisleft = numBytes >> 2 - (numWords);

is completely wrong, as thanks to operator precedence, it is equivalent to numBytes >> (2 - numWords) which is likely to be negative, causing undefined behaviour. Also, one has to be careful regardless, and keep in mind the differences between an arithmetic right shift and a logical right shift. Again, technically, right shifts for signed values are implementation defined, and could potentially be either.

share|improve this answer
1  
Your fix: int numWords = numBytes >> (sizeof(int) / 2); is wrong (try 8-bytes ints). It should be numBytes / sizeof(int) –  William Morris Dec 12 '13 at 17:32
    
@WilliamMorris You're 100% right, thanks. –  Yuushi Dec 12 '13 at 22:06
    
Should be / not >> –  William Morris Dec 13 '13 at 0:43
    
Fixed the bit count, but I'm a bit worried how many people get the right shift by two to be division by two when it in fact it's division by four. And by the fact that no one had noticed this in half a year. –  Emily L. Jul 8 at 19:53

On the update :

Notice the semi-colon at the end of the loop declaration :

for (i = 0 ; i < whatisleft; i++);

That's your syntax error : this loop does nothing since the semi-colon terminate it.

{
    *dst++ = *src++;


}

The part of code that should be the loop's body is valid even if it's not in the loop, and therefore is executed once. (The brackets are valid, moreover they induce a local scope what can taken advantage of ).

share|improve this answer
    
HaHa ,that's the typo ! +1 for the headsup :) –  Michael IV Dec 12 '13 at 14:15
2  
Where’s that a syntax error? Looks completely valid to me – algorithmically buggy, of course. –  Christopher Creutzig Dec 14 '13 at 20:29

Not mentioned elsewhere:

Source and destination can be pairwise unaligned

Which would force at least one unaligned write or unaligned read per word. Depending on platform this can cause a cpu exception, poor performance or be hardly noticeable.

Let the compiler do the job

A good optimising compiler will detect a memory copy and generate a fast sequence of instructions to do the copy. Just write a byte by byte copy and let the compiler worry about the details.

share|improve this answer
  • You are trying to re-assign a const.
  • bit shift operator should be switched (and the whole count bytes left-algorithm is off)
share|improve this answer
    
Why should it be switched?That calculation gives me number of bytes (chars) left after copy by word chunks is done. –  Michael IV Dec 12 '13 at 13:50
    
i mean >> should be switched to << as you said... didn't see that part of your post, sorry –  Max Dec 12 '13 at 13:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.