0
\$\begingroup\$

I'm writing this as an exercise. I would probably use a vector as a buffer internally (the extra capacity pointer overhead is not important enough). Primarily it's an exercise in writing copy/move constructors and assignment operators.

What are some of the pitfalls of defining these operators explicitly? What could go wrong?

#include <algorithm>
#include <span>

struct header_t { /* some fields... */ };
struct packet_t { /* some fields... */ };

const header_t default_header() { return header_t{ /* some init... */ }; }

class message_combiner {
    char* payload{nullptr};
    size_t size{0};
  public:
    std::pair<char*, size_t> get() const { return {payload, size}; };
    message_combiner(const std::span<packet_t>& data, const header_t& hdr = default_header())
        : size{sizeof(header_t) + data.size() * sizeof(packet_t)}
    {
        payload = new char[size];
        auto hdr_ptr = reinterpret_cast<header_t*>(payload);
        *hdr_ptr = hdr;
        auto data_ptr = reinterpret_cast<packet_t*>(hdr_ptr + 1);
        std::copy(data.begin(), data.end(), data_ptr);
    }
    message_combiner(const message_combiner& other) { *this = other; }
    message_combiner(message_combiner&& other) { *this = other; }
    message_combiner& operator=(const message_combiner& other)
    {
        size = other.size;
        payload = new char[size];
        std::copy(other.payload, other.payload + size, payload);
        return *this;
    }
    message_combiner& operator=(message_combiner&& other)
    {
        std::swap(size, other.size);
        std::swap(payload, other.payload);
        return *this;
    }
    ~message_combiner() { if (payload != nullptr) delete [] payload; }
    /// other functions that do useful things
};
\$\endgroup\$
2
  • 3
    \$\begingroup\$ This does not compile. You should preferably put up code that's working when you ask for a review. \$\endgroup\$
    – Ted Lyngmo
    Aug 26 at 19:29
  • \$\begingroup\$ Hm, I forgot about the “must be part of a concrete project” requirement when I reviewed this (I figured a learning project is a project, but this clearly isn’t a concrete project). Oh well. But I disagree with the “not working” objection: it’s wrong, for sure, but it could certainly appear to be working on many platforms. \$\endgroup\$
    – indi
    Aug 28 at 18:19
1
\$\begingroup\$

What are some of the pitfalls of defining these operators explicitly? What could go wrong?

The best code you can write is no code.

Every single line of code—every statement, every expression, every single character—introduces potential bugs, and needs to be inspected, tested, or both. The only possible way to have no potential for bugs is to not write any code.

That is why, whenever it is possible, you should let the compiler generate code for you. In theory, the compiler may have bugs… but even if it does, your final program will have more bugs if it has both the compiler bugs and the bugs you introduce. In practice, the compiler is much more rigorously tested, and regularly reviewed by many, many coders way, way better than you or I ever will be, so it will be a very, very rare thing to discover a bug in the compiler… whereas finding bugs in your own code will happen multiple times every day.

And, in point of fact, your implementation of these operators is riddled with bugs. Your code would be much better if you’d used something like std::vector, and let the copy/move operations be automatically generated. But even then, there are lot of problems with this class.

Code review

#include <algorithm>
#include <span>

You are using std::pair, but missing the <utility> header.

    char* payload{nullptr};

Okay, let’s set aside that you are using a naked pointer for ownership semantics for now. Even allowing for that, there are still piles of problems here.

You haven’t given nearly enough information about what this type is really supposed to be about, other than some vague, hand-wavey something-something about putting a header and a variable number of packets in it. What is message_combiner for? What is it supposed to do? Not knowing these things, I am forced to make guesses about what is going on here.

I see two possibilities for what you are trying to do.

  1. You want to copy the memory representation of a header_t and zero or more packet_t objects into a bunch of bytes. I can’t imagine why you would want to do this… it kinda looks like you might have some sort of data transmission in mind, but this would be wildly unportable. Depending on a lot of factors, not only will it be dangerous to share this data between different computers, it might not even work for sharing data between different processes on the same machine… hell, it might not even work for different processes of the same program compiled with the same compiler.

  2. You want to actually create real header_t and packet_t objects in a single memory buffer. There might be good reasons for this, like enforcing memory locality. But that’s more limited than you might think. And a lot harder to do right.

All in all, while what you’re doing looks like an absolutely terrible idea… and completely wrong… because you haven’t given enough information about what it’s supposed to be doing, I can’t be sure.

So I’ll just accept that you have a legitimate, sensible reason for this type. I seriously doubt you do, but I’ll give you the benefit of the doubt.

Okay, so you’re copying junk from memory into a byte array. For that, char* is the type you’d use… before C++20.

You are using C++20. In C++20, there is std::byte. This is now the correct type to use for this kind of thing.

So at the very least, this data member should be:

    std::byte* payload{nullptr};

But this is still terrible. I would refuse this in any of my projects, without even so much as a second glance.

std::vector<std::byte> should be your default choice for this, but if you don’t need the resizeability power of std::vector, you could go for std::unique_ptr<std::byte[]> instead. You’d have to manually keep track of the size, but if you’re not changing it after initialization, this isn’t a big problem.

Either way, not using a smart pointer or container of some kind is simply unacceptable in modern C++.

    std::pair<char*, size_t> get() const { return {payload, size}; };

This function smells like a terrible idea. First, is it really necessary to give everyone access to the buffer? Second, if it is necessary… why not use a span?

    message_combiner(const std::span<packet_t>& data, const header_t& hdr = default_header())
        : size{sizeof(header_t) + data.size() * sizeof(packet_t)}
    {
        payload = new char[size];
        auto hdr_ptr = reinterpret_cast<header_t*>(payload);
        *hdr_ptr = hdr;
        auto data_ptr = reinterpret_cast<packet_t*>(hdr_ptr + 1);
        std::copy(data.begin(), data.end(), data_ptr);
    }

Okay, let’s start at the top.

Never pass view types like std::span by const&. That’s just silly. The whole point of std::span is that it’s a cheap view of a span of data. It’s meant to be copied around. (Hopefully it will be passed in registers, but even if not, it will be passed by-value, avoiding unnecessary indirection, and the compiler can optimize aggressively because it doesn’t need to worry about aliasing.)

Personally, I am not a fan of default arguments. They cause way more problems than they’re worth. You would do better to have two constructors, with one delegating to the other:

    explicit message_combiner(std::span<packet_t> data)
        : message_combiner(data, default_header())
    {}

    message_combiner(std::span<packet_t> data, const header_t& hdr)
    // ...

Note also the explicit there. It’s not clear in your code because of the default arguments, but that constructor is potentially a converting constructor. Those almost always should be marked explicit.

Okay, now we get to the first UB bug in your code:

        auto hdr_ptr = reinterpret_cast<header_t*>(payload);
        *hdr_ptr = hdr;

So you cast your pointer to a header_t pointer, and then do a copy. The problem? You didn’t make sure that the ALIGNMENT of what payload points to matches the alignment of a header_t. If you’re not lucky, when you try to copy hdr into *hdr_ptr, it’s going to trigger a misaligned data fault, and… crash (if you’re lucky; with UB like this, you could get a whole lot worse than a mere crash).

The thing is, I don’t even know if aligning the payload properly is the right fix, because I can’t make sense of what you’re trying to do.

If your goal is just to copy the memory representation of a header_t… then do that. If your goal is to have your payload actually be a header_t (plus other stuff)… for whatever reason… then do that instead. They are two very, very different things.

        auto data_ptr = reinterpret_cast<packet_t*>(hdr_ptr + 1);

Why all the casting and pointer arithmetic? data_ptr is just payload + sizeof(header_t).

Now, casting that to a packet_t pointer creates the same problem as above: you haven’t guaranteed that the alignment is correct for packet_t. Which means:

std::copy(data.begin(), data.end(), data_ptr);

More UB.

I still don’t know what you think you’re doing here. Either you’re copying the memory representation of these objects into a byte array, or you’re trying to create a single chunk of memory that has an actual header_t followed by zero or more packet_t objects. Either option is… kinda silly, and totally unportable. So I can’t tell which flavour of silly you’re actually going for.

If you’re copying MEMORY REPRESENTATIONS, then you just have to do:

    // allocate the memory
    //
    // alignment doesn't matter if you're just copying the REPRESENTATIONS
    // of the objects
    payload = new std::byte[sizeof(header_t) + (sizeof(packet_t) * data.size())];

    // copy the header representation
    std::copy_n(static_cast<std::byte const*>(&hdr), sizeof(header_t), payload);

    // copy the representations of the packets, if any
    //
    // note that we just add the size of the header to skip past it
    std::copy_n(as_bytes(data), sizeof(packet_t) * data.size(), payload + sizeof(header_t));

Simple.

If you actually want to have ACTUAL OBJECTSnot just representations, but actual objects… things get much trickier.

  1. First, you need to calculate the size of the memory to allocation… and this is not trivial. To calculate the size, you have to:

    a. Start with the size of header_t. That’s the starting value, and the minimum size. b. If there are packets, then make sure the current value matches the alignment of packet_t. If not, increase the value until it does. c. Now add sizeof(packet_t) * data.size().

    As you can see, the really tricky part is in the middle, where you have to account for the alignment of packet_t.

  2. When you allocate the memory, you need to align it to the alignment of a header_t (because that will be the first object). So something like: payload = new (std::align_val_t{alignof(header_t)}) std::byte[/*size*/].

  3. You can just copy with *static_cast<header_t*>(payload) = hdr because it is now properly aligned.

  4. For the packets, you first need to find the offset to the first packet (if any). It’s the same logic as when calculating the size. Once you have the offset of the first packet, you can just do: std::uninitialized_copy(data.begin(), data.end(), static_cast<packet_t>(payload + offset)). Note that you have to use uninitialized_copy()… not copy(). Why? Because copy() only works when you are copying over existing objects. But you don’t have existing objects, you have raw, uninitialized memory.

  5. If anything throws an exception, you need to be able to handle it. The most dangerous point is after you have copied the header, because if anything else fails, it’s on you to destroy it.

This might look something like this:

    // allocate the memory
    //
    // alignment DOES matter; payload must be aligned as a header_t
    //
    // packet_offset is just the offset to the first packet, calculated as
    // described above
    if (data.empty())
        payload = new std::byte (std::align_val_t{alignof(header_t)}) [sizeof(header_t)];
    else
        payload = new std::byte (std::align_val_t{alignof(header_t)}) [packet_offset + data.size()];

    // if anything after this throws, you need to free the memory
    try
    {
        // copy the header
        *static_cast<header_t*>(payload) = hdr;

        // if anything after this throws, you need to destroy the header
        try
        {
            if (not data.empty())
                std::uninitialized_copy(data.begin(), data.end(), static_cast<packet_t*>(payload + packet_offset));
        }
        catch (...)
        {
            std::destroy_at(static_cast<header_t*>(payload));
        }
    }
    catch (...)
    {
        delete[] payload;

        throw;
    }

That is UGLY, largely because payload is not a smart pointer.

Note that if you go this route, you are COMPLETELY responsible for managing the header and packet objects. That means you have to manually delete them in the destructor, manually copy them in the copy constructor, and so on. (If you are just working with representations, you don’t need to worry about that stuff.)

    message_combiner(const message_combiner& other) { *this = other; }

This is a bad way to do copy construction. It is not just misguided, it is inefficient.

In order to use copy assignment, you must have a fully-constructed object. So if you wanted to do it this way you would first have to properly construct the object… and then do the assignment:

    message_combiner(message_combiner const& other)
        : message_combiner() // default construct this object first
    {
        // now that you have a fully (default) constructed object, you can assign
        *this = other;
    }

Right now, your copy constructor “works” because the member initializers roughly approximate a default consturctor. But that’s just for now… if you change the class, that may no longer be true.

If you do copy construction this way, you are completely constructing an object… and then immediately obliterating it by copying over it. That’s silly. That’s why C++ coders generally do it the other way around: they write a proper copy constructor, then write copy assignment in terms of that.

    message_combiner(message_combiner&& other) { *this = other; }

All the same problems as the copy constructor, plus more.

First, that should be a move assignment.

Second, move ops should be noexcept wherever possible. (And that’s certainly possible here.)

    message_combiner& operator=(const message_combiner& other)
    {
        size = other.size;
        payload = new char[size];
        std::copy(other.payload, other.payload + size, payload);
        return *this;
    }

First, you are failing to delete the existing payload before allocating a new one.

Second, everything else in the function is wrong, because it should all be more or less identical to regularly constructing the object.

In theory, copy assignment is just:

  1. destruction; followed by
  2. copy constructing over the ashes of the old object.

That’s the pattern you are attempting to write. But that’s a dangerous pattern, because if anything fails in the second step, you have already destroyed the original. Your object, and likely your program by extension, will now be in a broken state.

A safer pattern is the copy-and-swap pattern:

    auto operator=(message_combiner const& other) -> message_combiner&
    {
        auto temp = other; // copy

        std::ranges::swap(*this, temp); // swap - normally this will be no-fail

        return *this;
    }

If the copy fails, then the original object is untouched.

This is why you should write the copy constructor properly, and then do copy assignment in terms of copy construction… not the other way around.

    message_combiner& operator=(message_combiner&& other)
    {
        std::swap(size, other.size);
        std::swap(payload, other.payload);
        return *this;
    }

This is fine, but it could be noexcept.

Also, you should really consider writing a swap function, and then writing move assignment (and copy assignment, and move construction) in terms of that. If you do it the way you are doing now, then swapping becomes ridiculously over-complicated. On the other hand, if you write a proper swap, then that will be efficient… and everything that uses swapping will also be.

    ~message_combiner() { if (payload != nullptr) delete [] payload; }

If you are just storing representations of the header and packets, then this is fine.

But the way you’ve written the code, where you actually create real header and packet objects within the memory payload points to, this is not good enough. If you have real header and packet objects, they need to be destroyed. You can’t just delete the memory out from under them.

Summary

There is a lot of conceptual confusion here: quite frankly, I don’t think you really know what you’re doing.

You can’t just copy the memory representation of objects around willy-nilly. This is C++; not C. (And even in C, the way you’re copying representations around would be clumsy and ill-formed.) Objects should be treated like actual objects, not just a bag of bytes in memory. They should be properly constructed, and they should be properly destroyed. Even if the constructor and destructor are no-ops, which is often the case for simple types, you still have to treat them like they do actual stuff.

So you can’t just allocate a chunk of memory and then copy objects into it. You either have to first initialize that memory properly (by constructing objects in it with placement new, for example), or you have to use the uninitialized memory algorithms (which basically just use placement new or construct_at() under the hood).

And even if you could just allocate a chunk of memory and copy objects into it, you’d still need to respect things like alignment.

And even if you managed to fix all the problems here, one way or the other—either by just using memory representations, or by properly initializing real objects in the uninitialized memory and then managing them properly—there doesn’t seem to be any point to it all, because you couldn’t really do anything with the class. It’s useless; the whole idea behind it is misguided. Whether it’s memory representations or actual objects, it won’t work for data transfer, and would be really dodgy for serialization. As I said, I don’t think you really know what you’re doing.

You said your focus was on the copy and move operations. Well, they’re all wrong, but I can’t even tell you how to fix them properly, because I can’t make sense of what you’re trying to do. But you basically have two options:

  1. If you are just storing the memory representations of a header and a bunch of packets, then your copy/move ops are close to correct. There are some things that need fixing, but you have the general idea right.

  2. If you are storing actual objects… then no, all of your copy/move ops are just tragically wrong; not even close to correct. What you would have to do in this case is manually handle the copying/moving of every object in your payload. For example, to copy a message_combiner, you would first have to allocate the same amount of memory, and then manually copy the header, and then manually copy all the packets. And in your destructor, you would have to manually destroy the header, and then manually destroy all the packets, and then free the memory. All of that is a lot of work.

Generally, the copy/move ops should be done like this:

class whatever
{
public:
    // ... other stuff in the class ...

    whatever(whatever const& other)
    {
        // properly copy-construct from other
    }

    whatever(whatever&& other) noexcept : whatever{}
    {
        std::ranges::swap(*this, other);
    }

    auto operator=(whatever const& other) -> whatever&
    {
        auto temp = other;
        std::ranges::swap(*this, temp);
        return *this;
    }

    auto operator=(whatever&& other) noexcept -> whatever&
    {
        std::ranges::swap(*this, other);
        return *this;
    }

    friend auto swap(whatever& a, whatever& b) noexcept
    {
        // for each data member:
        std::ranges::swap(a./*...*/, b./*...*/);
    }
};

You just need to write:

  1. a proper (cheap, no-fail) default constructor (or properly initialize to some default state in the move constructor before swapping)
  2. a proper copy constructor; and
  3. a proper swap function (usually just a bunch of swaps of the data members)

That’s the general pattern. The only places the details really change are in the copy constructor. Everything else is more-or-less boilerplate.

If your ultimate goal is to sent messages across the wire, then you are barking up the completely wrong tree. You need to look at proper serialization of types, which is not just memcpy()ing the memory representations.

I guess the bottom line is this: If you really want to learn how to write proper copy/move operations, you should first start with something that you understand a little better, and write good copy/move ops for that. (And that is not necessarily easy! Writing good copy/move ops can be hard.) Trying to learn how to write good copy/move ops for such a muddled, incoherent idea as this… you’re not helping yourself. Master one thing at a time; start with simple types, and learn how to write copy/move ops for them… and then consider moving on to more complex types, like this… whatever this “message_combiner” is actually supposed to be.

Questions

Usage of std::unique_ptr<std::byte[]>

std::unique_ptr<std::byte> would be a pointer to a single byte:

auto p = std::unique_ptr<std::byte>{new std::byte{}};
// or
auto p = std::unique_ptr{new std::byte{}};
// or
auto p = std::make_unique<std::byte>();

// or, if you want default initialization:
auto p = std::unique_ptr<std::byte>{new std::byte};
// or
auto p = std::unique_ptr{new std::byte};
// or
auto p = std::make_unique_for_overwrite<std::byte>();

And the usage would be pretty much the same as for any pointer:

// get the value
auto val = *p;

// set the value
*p = std::byte(42);

// and so on
if (p != nullptr) ...

std::unique_ptr<std::byte[]> would be a pointer to an array of bytes:

// allocates an array of 100 value-initialized bytes
auto p_bytes = std::unique_ptr<std::byte[]>{new std::byte[100]{}};
// or
auto p_bytes = std::make_unique<std::byte[]>(100);

// or, if you want default initialization:
auto p_bytes = std::unique_ptr<std::byte[]>{new std::byte[100]};
// or
auto p_bytes = std::make_unique_for_overwrite<std::byte[]>(100);

And the usage would be pretty much the same as for any pointer-to-array:

// get the 33rd element's value
auto val = p_bytes[33];

// set the 33rd element's value
p_bytes[33] = std::byte(42);

// get the pointer to the start of the array
auto p_begin = p_bytes.get();
auto p_end = p_begin + 100;

Let’s assume payload is defined like this:

std::unique_ptr<std::byte[]> payload = nullptr;

Then your constructor might look like:

message_combiner(std::span<packet_t> data, header_t const& hdr = default_header())
{
    // this could be a private class constant
    constexpr auto packet_offset = /* calculate offset to first packet somehow */;

    // determine the size
    if (data.empty())
        size = sizeof(header_t);
    else
        size = packet_offset + (data.size() + sizeof(packet_t));

    // allocate (note alignment is handled)
    payload = new (std::align_val_t(alignof(header_t))) std::byte[size];

    // if there are any errors after this, no problem, unique_ptr will
    // automatically free the memory

    // construct the header
    std::construct_at(static_cast<header_t*>(payload.get()), hdr);

    try
    {
        // construct the packets
        std::ranges::uninitialized_copy(data,
            std::span{static_cast<packet_t*>(payload.get() + packet_offset), data.size()});
    }
    catch (...)
    {
        // you have to manually destroy the header
        std::destroy_at(static_cast<header_t*>(payload.get()));

        throw;
    }
}

Which, as you can see, aside from the .get()s, is no different from when payload is a naked std::byte*… except that there’s one less try-catch level, because unique_ptr will automatically clean itself up. (Unfortunately, without something like scope_fail, you can’t avoid the try-catch block to clean up the header.)

What does a proper copy constructor look like?

The answer depends on your class.

The general form of the move constructor, move assignment, and copy assignment don’t change from class to class. The default implementations will always be right (though may not be the most efficient, in some rare scenarios, like if you’re expecting a lot of self-assignments… or, of course, if they could have been left default-generated).

The copy constructor, however, is very specific for each class. If it can’t be default-generated, then it will usually be the hardest part of all the fundamental operations to write.

For example, in your case, what you’d have to do is:

  1. allocate the memory (which will be the same size as other’s memory allocation)
  2. std::construct_at() the header as a copy of other’s header; then
  3. std::uninitialized_copy() the packets.

This is basically the same as the regular constructor. Indeed, you could write the copy constructor as:

// assuming you have the following member functions:
//  *   header(), which returns a const& to the header in the payload
//  *   packets(), which returns a span<packet_t> view of the packets, if any

message_combiner(message_combiner const& other)
    : message_combiner{other.packets(), other.header()}
{}

This happens to work well for this class, but for other classes the same pattern will sometimes be very inefficient (or really silly to have a constructor that makes it possible, because it would be exposing internal stuff).

Won’t *this = other; get optimized to *this = std::move(other);?

No. x = y will never get optimized to x = std::move(y).

To see why this would be a terrible idea, imagine if your move constructor wanted to do something with other after the assignment:

message_combiner(message_combiner&& other) noexcept
    : message_combiner{}
{
    *this = other;

    do_something(other); // oops, other was silently moved away from
}

Compare that to this:

message_combiner(message_combiner&& other) noexcept
    : message_combiner{}
{
    *this = std::move(other);

    do_something(other);    // other was moved away from... but you can
                            // clearly see that, so this mistake is easy to
                            // spot
}

You could argue that the compiler can “see” whether other will be used after the assignment, and if not, then it’s safe to move. But that would be sketchy at least, because moving and copying may do very different things (I mean, you would be foolish if you made a class that did that… but people do a lot of foolish things in C++, and the language and compiler need to account for that). To have exactly the same code do different things depending on stuff that happens elsewhere would be daft (yes, people write code that does that sometimes… but it’s not a good idea).

You could also think of it like this: moving an object is basically destroying it, so it should never happen invisibly. It should always be crystal clear when you’re ripping the guts out of an object, like with an explicit move(), or when it’s being destroyed anyway, like when you’re returning an object (which can never be used after, so really is safe to always move and never copy).

To understand what’s happening, remember that the easiest way to distinguish between an lvalue and an rvalue is that if you can take the address of something, it’s an lvalue; if not, it’s an rvalue.

Can you take the address of other? Of course:

message_combiner(message_combiner&& other) noexcept
    : message_combiner{}
{
    if (this != &other) // silly because it will never be false... but you can do it, so it illustrates the point
        *this = other;
}

Since you can take the address of other, other is an lvalue. So *this = other is an lvalue assignment… that is, a copy assignment.

(Another trick some people use to distinguish lvalues and rvalues is: if it has a name, it’s an lvalue… otherwise it’s an rvalue. other has a name—that name is “other”—so it’s an lvalue.)

If you want a move assignment, you need to cast other to an rvalue, which is what std::move() does.

\$\endgroup\$
2
  • \$\begingroup\$ A few follow ups... could you elaborate on the usage of std::unique_ptr<std::byte[]>? what does a proper copy constructor look like? you were pretty explicit with the move ctor. Also, disregarding the other issues, wont *this = other; get optimized to *this = std::move(other); anyway? \$\endgroup\$
    – Payal
    Aug 27 at 8:03
  • \$\begingroup\$ The answers take a bit of explanation, so I’ll extend the answer above, and add them there. \$\endgroup\$
    – indi
    Aug 28 at 19:36

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