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
    Packet(char* data, int length) : mLength{length}
        mData = new char[mLength];
        memcpy(mData, data, length);

        delete [] mData;
    int mLength;
    char* mData = nullptr;

class Key
    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;

    int mCodeType1 = 0;
    int mCodeType2 = 0;

class Value
    Value(char* data, int length) : sharedPacketPtr{new Packet(data, length)} {}
    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;

  • 2
    \$\begingroup\$ I think this question is off-topic for this site; you are asking for best practices and you are showing some stub code that doesn't do anything useful. Code Review is for concrete code that is known to work. \$\endgroup\$
    – G. Sliepen
    Jun 2 at 14:23
  • 4
    \$\begingroup\$ Welcome to Code Review! Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Jun 2 at 14:58

1 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().

  • \$\begingroup\$ Thank you for your precious comment. I forget to delete third argument. \$\endgroup\$
    – unique
    Jun 2 at 14:51

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