12
\$\begingroup\$

Here is my implementation of memmove; where can I improve?

void* memmove(void* dest, const void* src, std::size_t count)
{
    char *dest_ = static_cast<char*>(dest), *src_ = (char*)(src);
    if ((char*)src + count > dest && src < dest) //
    {
        dest_ += (src_ += count - 1, count - 1);
        while (count--) *dest_-- = *src_--;
    }
    else while (count--) *dest_++ = *src_++;
    return dest;
}
\$\endgroup\$
  • \$\begingroup\$ I'll just leave a link to interesting video about C style casts. Not a secret, but when in a video, is much more engaging. \$\endgroup\$ – Incomputable Sep 6 '17 at 13:24
  • 10
    \$\begingroup\$ The biggest improvement is not to use it after you are done with the exercise. Remember why the question is tagged as it is. \$\endgroup\$ – StoryTeller Sep 6 '17 at 16:34
  • \$\begingroup\$ If you use unsigned char rather than char, your function will be guaranteed safe if some of the source bytes are indeterminate. \$\endgroup\$ – aschepler Sep 7 '17 at 1:19
18
\$\begingroup\$

Missing header: #include <cstddef> is required for std::size_t (other headers also provide it).

There's no need for src_ to cast away the constness of *src:

char *dest_ = static_cast<char*>(dest);
char const *src_ = static_cast<char const*>(src);

Personally, I'd just go for d and s rather than the ugly trailing underscores, but that's very much a matter of taste.

Arithmetic and comparison between pointers to different objects is unspecified behaviour, and I don't see a way to avoid it in the test. Thankfully, in the case where src and dest are in different objects, it doesn't matter which branch you take

There's no need to test for src + count > dest as well as src < dest; we can quite happily use the "reverse" copy for any lower-to-higher copy, as long as we don't subtract from count==0 in that branch.

In the "reverse" branch, it's disingenuous to hide the assignment to src_ inside the assignment to dest_. Much clearer to write as:

    src_ += count - 1;
    dest_ += count - 1;

We can avoid that -1 if we pre-decrement rather than post-decrement (it's safe to point to one past the end).

Style: Use braces on both sides of else, or neither; avoid if () {} else ;.


My version

#include <cstddef>

void *memmove(void *dest, const void *src, std::size_t count)
{
    auto d = static_cast<char*>(dest);
    auto s = static_cast<char const*>(src);

    // If s and d are in distinct objects, the comparison is
    // unspecified behaviour, but either branch will work.
    if (s < d) {
        s += count;
        d += count;
        while (count--)
            *--d = *--s;
    } else {
        while (count--)
            *d++ = *s++;
    }

    return dest;
}

Performance

Copying a single char at a time is going to be sloowww. Real implementations take advantage of processors' native transfer size, DMA hardware, and specialized instructions where available. That's why it's a standard library function rather than user-supplied.

\$\endgroup\$
  • \$\begingroup\$ Can you add a link to a common library implementation (edit: source code) that has hardware specific code paths? I'm sure this exists but am having a hard time getting good search results. \$\endgroup\$ – BurnsBA Sep 6 '17 at 12:23
  • 4
    \$\begingroup\$ You know about Enhanced REP MOVSB on x86? Relying on the microcode can be superior to calling a library function. \$\endgroup\$ – Deduplicator Sep 6 '17 at 16:19
  • 5
    \$\begingroup\$ std::less yields a total order for any pointer type, even if < doesn't, so it may be preferable to use std::less to make it easier to reason about the program's correctness. \$\endgroup\$ – user2357112 Sep 6 '17 at 19:38
  • 2
    \$\begingroup\$ @BurnsBA: here's glibc's memmove/memcpy implementation for x86-64, written in assembly (AT&T syntax). The design-notes comment is pretty good, explaining the strategy for different sizes. For short copies, it uses two potentially-overlapping loads from the start & end, then two stores. This always works regardless of src/dst overlap, so it avoids checking anything but size for small copies, and is very good. \$\endgroup\$ – Peter Cordes Sep 7 '17 at 1:09
  • 1
    \$\begingroup\$ Saw this again while looking for the link in my last comment :P GCC's builtin memcpy inlines small sizes, but for sizes that aren't known at compile time it (almost?) always calls the library memcpy. Possibly with profile-guided optimization you might see it choose to inline some code when the size at runtime turns out to always be small, but it's a good example of why letting the compiler see constants is very helpful. \$\endgroup\$ – Peter Cordes Mar 27 '18 at 6:34
10
\$\begingroup\$

Comparing pointers to unrelated objects has unspecified result.

Do not use C-style casts, or use them consistently everywhere. Instead, use a static_cast<char*>(const_cast<void*>(src)).

Do not use multiple assignments in a single statement (dest_ += ...), especially not in combination with the comma operator. This is only confusing and helps neither the human reader nor the compiler.

Remove the empty comment. It doesn't serve any purpose.

Instead of adding count - 1, you can add count and use pre-decrement. This is clearer code since you don't have a magic 1 lying around.

The cast in the condition can be replaced by just src_.

The whole condition should read (src_ < dest_ && dest_ < src_ + count) to match the common pattern for a between test.

\$\endgroup\$
  • 1
    \$\begingroup\$ I mistakenly claimed that the pointer comparison was undefined, but stole a correction from you - thanks and +1. \$\endgroup\$ – Toby Speight Sep 6 '17 at 11:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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