Is memcpy with this pointer safe?

Question

I'm currently writing an own string implementation in C++. (Just for exercise).

However, I currently have this copy-constructor:

// "obj" has the same type of *this, it's just another string object
string_base<T>(const string_base<T> &obj)
        : len(obj.length()), cap(obj.capacity()) {
    raw_data = new T[cap];
    for (unsigned i = 0; i < cap; i++)
        raw_data[i] = obj.data()[i];
    raw_data[len] = 0x00;
}

and I wanted to increase performance a little bit. So I came on the idea using memcpy() to just copy obj into *this.

Just like that:

// "obj" has the same type of *this, it's just another string object
string_base<T>(const string_base<T> &obj) {
     memcpy(this, &obj, sizeof(string_base<T>));
}

Is it safe to overwrite the data of *this like that? Or may this produce any problems?

Thanks in advance!

This thread makes for interesting reading; nearly a duplicate: https://stackoverflow.com/questions/30114397/constructing-a-trivially-copyable-object-with-memcpy — Bathsheba, Jun 18 '18 at 10:23
you can also use strcpy which stops at a NULL character. It may differ in terms of performance compared to memcpy — MauriceRandomNumber, Jun 18 '18 at 10:25
Another thing to consider is that modern compilers are often smart enough to recognize the loop in the first example and replace it with memcpy when appropriate. So this might be a premature optimization. — Bo Persson, Jun 18 '18 at 10:25

Daniel Langr · Answer 1 · 2018-06-18T10:26:21.940

3

No, it's not safe. From cppreference.com:

If the objects are not TriviallyCopyable, the behavior of memcpy is not specified and may be undefined.

Your class is not TriviallyCopyable, since its copy constructor is user-provided.

Moreover, your copy constructor would make only shallow copies (which might be fine if you wanted, e.g., copy-on-write mechanism applied with your strings).

edited Jun 18 '18 at 10:26

answered Jun 18 '18 at 10:23

Daniel Langr

22,196
3
50
93

What does "TriviallyCopyable" mean ? – cocoz1 Jun 18 '18 at 10:24
1

I'm not sure that being trivially copyable is sufficient, although it is necessary. – Bathsheba Jun 18 '18 at 10:25
1

@codekaizer Not at all: `std::cout << std::is_trivially_copyable::value << std::endl;`. (Again, `std::string` has user-defined copy constructor.) – Daniel Langr Jun 18 '18 at 10:50
@DanielLangr, yeah, had just read [this answer](https://stackoverflow.com/a/30114645/8012206). Based on the comment - *std::string is not trivially copyable in c++14 and onwards* – Joseph D. Jun 18 '18 at 10:51
1

@codekaizer No, read the very next comment: "_An object containing a `std::string` has never been trivially copyable"_. – Daniel Langr Jun 18 '18 at 10:53

Attersson · Answer 2 · 2018-06-18T10:57:35.927

2

It will produce problems. All references and pointers will be just copied, even the pointer to raw_data, which will be the same as the source object.

As a requisite in order to use memcpy, your class should:

Be Trivially Copyable
Have no references or pointers unless: static pointers or not owning the pointed data. Unless you know what you are doing such as implementing a copy-on-write mechanism or when this behaviour is otherwise intended and managed.

edited Jun 18 '18 at 10:57

answered Jun 18 '18 at 10:23

Attersson

4,755
1
15
29

Pointers are fine if they do not own the data they are pointing to. Otherwise, we get multiple deletion of the same memory. – Aconcagua Jun 18 '18 at 10:46
Exactly... what @Aconcagua said. Edited. – Attersson Jun 18 '18 at 10:52
Sure, they may be reference-counted as well. As for copy-on-write mechanism. That's why I would be careful with such a strong statement. – Daniel Langr Jun 18 '18 at 10:54
1

Well I edited again but this should be it. The caution about references and pointers is mandatory – Attersson Jun 18 '18 at 10:58
If you have reference members, and you do a `memcpy`, it means you are rebinding a reference. It's illegal. – curiousguy Jun 18 '18 at 13:16
Do you think I should put a comma? "Have no references , (or pointers unless...)" ? – Attersson Jun 18 '18 at 13:19

score 1 · Answer 3 · answered Jun 18 '18 at 11:33

As others have said, in order for memcpy to work correctly, the the objects being copied must be trivially copyable. For an arbitrary type like T in a template, you can't be sure of that. Sure, you can check for it, but it's much easier to let somebody else do the checking. Instead of writing that loop and tweaking it, use std::copy_n. It will use memcpy when that's appropriate, and element-by-element copying when it isn't. So change

raw_data = new T[cap];
for (unsigned i = 0; i < cap; i++)
    raw_data[i] = obj.data()[i];

to

raw_data = new T[cap];
std::copy_n(obj.data(), cap, raw_data);

This also has the slight advantage of not evaluating obj.data() on every pass through the loop, which is an optimization that your compiler might or might not apply.

score 0 · Answer 4 · answered Jun 18 '18 at 11:28

It seems pretty obvious from the limited fragment that raw_data is a member, and a pointer to a new[]'ed array of T. If you memcpy the object, you memcpy this pointer. You don't copy the array.

Look at your destructor. It probably calls delete[], unconditionally. It has no idea how many copies exist. That means it's calling delete[] too often. This is fixable: you'd need something similar to shared_ptr. That's not at all trivial; you have to worry about thread safety of that share count. And obviously you can't just memcpy the object, as that would not update the share count.

Is memcpy with this pointer safe?

4 Answers4