Copying allocated data into std::map in a smart way [closed]

Question

I come across a problem and I solved it. The solution works but I have some feelings that there is something wrong with my solution/code.

To be clear, let's assume that cars on the race track transmit their velocity periodically. I want to store only the final velocity data of each car in a std::map where the key represents the car id and the value represents the velocity. However, velocity is not an integer but an allocated memory that belongs to some other code and it is deleted automatically after reading. So, when a car transmits its velocity data I want to copy allocated data and insert it into a map with a key.

While inserting (key1, data1) pair into the std::map, if there is no element with "key1" then the code should simply copy data and insert it into the std::map. However, if there is an element with a key "key1" then the code should delete allocated memory in std::map and copy the final data into the map.

I solved my problem using a shared pointer to allocate/deallocate memory easily as below. However, I think this solution is complex and I believe that there is an easier solution. Do you have any idea?

#include <iostream>
#include <map>
#include <memory>
#include <cstring>

class Packet
{
public:
    Packet(char* data, int length) : mLength{length}
    {
        mData = new char[mLength];
        memcpy(mData, data, length);
    }

    ~Packet()
    {
        delete [] mData;
    }
private:
    int mLength;
    char* mData = nullptr;
};

class Key
{
public:
    Key(int codeType1, int codeType2, int codeType3):
        mCodeType1{codeType1}, mCodeType2{codeType2} {}

    inline bool operator<(const Key& rhs) const
    {
        if(mCodeType1 < rhs.mCodeType1)
            return true;
        if(mCodeType1 == rhs.mCodeType1 && mCodeType2 < rhs.mCodeType2)
            return true;

        return false;
    }

private:
    int mCodeType1 = 0;
    int mCodeType2 = 0;
};

class Value
{
public:
    Value(char* data, int length) : sharedPacketPtr{new Packet(data, length)} {}
private:
    std::shared_ptr<Packet> sharedPacketPtr;
};

int main()
{
    std::map<Key, Value> myMap;

    const int dataLength = 1000; // const is just for demonstration
    //data 1,2,3 comes from somewhere else (I can't change data type)
    char* data1 = new char[dataLength];
    char* data2 = new char[dataLength];
    char* data3 = new char[dataLength];

    myMap.insert(std::make_pair(Key{1, 1, 1}, Value(data1, dataLength)));
    myMap.insert(std::make_pair(Key{1, 1, 1}, Value(data2, dataLength)));
    myMap.insert(std::make_pair(Key{3, 3, 3}, Value(data3, dataLength)));

    // data 1,2,3 delete themselves (no need to worry)
    delete [] data1;
    delete [] data2;
    delete [] data3;

    return 0;
}

Answer 1

If a Packet is copied, then we'll have two objects with the same value of mData. This will mean that we delete[] this pointer twice, which causes Undefined Behaviour. Instead of this class, can't we just use a std::vector, which will manage memory properly with no work on our part? That looks much easier:

struct Packet
{
    std::vector<char> mData;

    Packet(char const* data, std::size_t length)
        : mData{data, data+length}
    {}
};

That quietly fixes the above issue (and also the misspelling of std::memcpy, which is no longer needed).

I don't see why we need three arguments to construct a Key, given the third one is ignored.

Key::operator<() looks like it could simply be defined = default:

    auto operator<=>(const Key& rhs) const = default;

There's no real benefit from the extra level of indirection introduced by the std::shared_ptr. That would be useful only if we intended to share ownership of the objects in the map. Even if we did, creating a class should be unnecessary - just use plain old std::make_shared().