Is this the best way to use unique_ptr to avoid leaks?

Question

I'm a beginner and I am working to understand memory leaks.

Does the code below have any leaks? Does it make sense to use unique_ptr for the vector and map?

My purpose is mostly for performance and minimal tolerance in my code. If this code is wrong, please tell me where I made a mistake.

typedef struct MyStruct
{
    std::string name;
    int age;
} tMyStruct;

typedef std::map<std::unique_ptr<MyStruct>, std::unique_ptr<MyStruct>> den_mp;
typedef std::vector< std::unique_ptr<MyStruct> > den_vec;

den_mp  m_mp;
den_vec m_vec; 

void doit(auto& mp)
{
    for (auto it = mp.begin(); it != mp.end(); it++)
    {
        std::cout << it->first->name << std::endl;
    }
}

void doit2(auto& mp)
{
    for (auto it = mp.begin(); it != mp.end(); it++)
    {
        std::cout << it->get()->name << std::endl;
    }
}

void addvec(std::string name, int age)
{
    std::unique_ptr<MyStruct> strt(new MyStruct);
    strt->age = age;
    strt->name = name;
    m_mp.emplace(std::move(strt), std::move(strt));
}

void addvec2(std::string name, int age)
{
    std::unique_ptr<MyStruct> strt(new MyStruct);
    strt->age = age;
    strt->name = name;
    m_vec.emplace_back(std::move(strt));
}

int main()
{
    addvec("ads", 10);
    addvec2("asdfg", 10);

    doit(m_mp);
    doit2(m_vec);

    std::cin.get();
}

It looks like you have a map that only ever maps instances of `MyStruct` to itself. It sounds like you may want `std::set>` instead. — François Andrieux, Jul 07 '21 at 18:01
This doesn't address the question, but in C++ you don't have to do the `typedef struct ... struct_name;` dance. `struct myStruct { ... };` works just fine. That typedef stuff is a C habit, because of the peculiar way that struct names are treated in C. — Pete Becker, Jul 07 '21 at 18:06
@CILGINIMAM As an aside note, you may prefer writing `auto strt{std::make_unique()};` instead of `std::unique_ptr strt(new MyStruct);` and `for (auto it : mp)` instead of `for (auto it = mp.begin(); it != mp.end(); it++)`; or better yet `for (const auto& it : mp)`. — rturrado, Jul 07 '21 at 18:08

score 4 · Answer 1 · answered Jul 07 '21 at 17:55

4

Is the codes have any leak?

No, the shown program doesn't have memory leaks as far as I could tell.

However, this is broken:

 m_mp.emplace(std::move(strt), std::move(strt));

If you move from something twice, then the second move will be from an object that has already been moved from. In this case, the unique pointer would be null after being moved from.

You cannot have multiple unique pointers pointing to the same object. That's why it's called "unique".

Does it make sense to use unique_ptr vector and map?'

It can. But it doesn't always make sense. It can be unnecessarily inefficient when storing the object itself as an element would suffice.

answered Jul 07 '21 at 17:55

eerorika

232,697
12
197
326

@CILGINIMAM Just add a `std::cout << it->second->name << std::endl;` line after `std::cout << it->first->name << std::endl;` in `doit` to test `it->second` is null and you get a bang! – rturrado Jul 07 '21 at 17:59
@eerorika thank u my friend, when i need add key and value what should I do? – CILGINIMAM Jul 07 '21 at 19:41

Hanjoung Lee · Answer 2 · 2021-07-07T18:05:37.340

Is the codes have any leak?

No.

Does it make sense to use unique_ptr vector and map?

Sure. But you should be aware of what you are doing. Especially using unique_ptr as the key type of a map.

If this codes is wrong pls tell me where I made a mistake?

void addvec(std::string name, int age)
{
        std::unique_ptr<MyStruct> strt(new MyStruct);
        strt->age = age;
        strt->name = name;
        m_mp.emplace(std::move(strt), std::move(strt)); // Undefined Behavior
}

m_mp.emplace(std::move(strt), std::move(strt))

Let's say the first move is called first. It is moved successfully(as key), then the second move would be evaluated to null, as it has been already moved
According to C++ standard, you cannot be sure which argument is evaluated first. i.e. the second move could be called first. Order of evaluation in C++ function parameters

score 0 · Accepted Answer · answered Jul 07 '21 at 18:13

0

The code is overly elaborate. If you just write the obvious it will work:

std::vector<MyStruct> m_mp;

void addvec2(std::string name, int age) {
    m_mp.emplace_back(name, age);
}

There is no need for pointers here. The best way to avoid leaks is to not make heap allocations.

Of course, if the code in the question was a simplification of a problem that actually required heap allocations, ignore this.

answered Jul 07 '21 at 18:13

Pete Becker

74,985
8
76
165

thanks for comment! btw when i need clear map elements i need just clear or i need clear and it->relase(); ? – CILGINIMAM Jul 07 '21 at 19:32

Is this the best way to use unique_ptr to avoid leaks?

3 Answers3