Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

closed as off-topic by Jamal Jun 20 at 22:32

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the 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
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
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 '14 at 19:49
You're not using x86 or x86-64 at all? They all have sizeof(int) == 4... – Emily L. Jul 8 '14 at 20:07
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
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 '14 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
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

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