Add an element to a vector inside a struct, from within another struct

Question

This is most probably trivial and I'm confusing struct allocation and pointers somehow, I apologize for this. I have read answers to similar questions but it didn't help. The code is, as always, way more complicted, this is a reduction from 3000+ lines of code to the gist.

The output I expected was

prep 1
main 1

Instead, I get

prep 1
main 0

This is the code:

#include <iostream>
#include <vector>

using namespace std;

struct Entry
{
    vector<int> list;
};

struct Registry
{
    vector<Entry> entries;

    void prep()
    {
        Entry* entry = new Entry();
        entries.push_back(*entry);
        entry->list.push_back(0);
        cout << "prep " << entry->list.size() << "\n";
    }
};

int main()
{
    Registry registry;
    registry.prep();
    cout << "main " << registry.entries[0].list.size() << "\n";

    return 1;
}

@TedLyngmo I don't know the number of items beforehand, it's a parser that constructs a tree as it traverses the textual input file. As you've all noticed, I don't know C++ very well. I'd allocate the struct in the function statically, but on exit isn't it remove from the stack? That's why I use new: I need it on the heap and survive function exit. Please correct me where I'm wrong :) — pid
– pid, Commented Apr 20, 2021 at 11:12
@pid You dereference the dynamically allocated entry and copy it into the vector, which then controls the lifetime of that new element. You can just as well let the vector create the new element and avoid the pointless copy, not to mention in your case leaking the original. — underscore_d
– underscore_d, Commented Apr 20, 2021 at 11:23

Ted Lyngmo · Accepted Answer · 2021-04-20 11:17:24Z

2

You don't store pointers in your vector<Entry> so you should not use new. Instead add a default constructed Entry using emplace_back.

A C++17 approach:

void prep()
{
    Entry& entry = entries.emplace_back(); // returns a ref the added element
    entry.list.push_back(0);
    cout << "prep " << entry.list.size() << "\n";
}

Prior to C++17:

void prep()
{
    entries.emplace_back(); // does NOT return a ref the added element
    Entry& entry = entries.back(); // obtain a ref to the added element
    entry.list.push_back(0);
    cout << "prep " << entry.list.size() << "\n";
}

If you do want to create and maniplate your Entry before adding it to entries, you can do that too and then std::move it into entries.

void prep()
{
    Entry entry;
    entry.list.push_back(0);
    cout << "prep " << entry.list.size() << "\n";
    entries.push_back(std::move(entry)); // moving is a lot cheaper than copying
}

edited Apr 20, 2021 at 11:17

answered Apr 20, 2021 at 11:11

Ted Lyngmo

125k7 gold badges93 silver badges156 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

pid Over a year ago

Thank you very much! I was banging my head for hours :) This has solved several problems in the code with similar patterns. Two more questions please: do I have to cleanup the entries as soon as I'm done processing (to remove the leak)? And on program exit, do the allocations remain and fragment the heap or does C++17 (clang++-12) know the owning process' PID and thus cleanup allocated resources?

Ted Lyngmo Over a year ago

You're welcome! If you use standard containers and no manual memory management (no new/delete or new[]/delete[]s), it will be cleaned up automatically. So, in the code in my answer, there will be no leaks. When the program terminates, the objects in these containers will be destroyed properly too, so no need to worry about leaks.

zerocukor287 · Accepted Answer · 2021-04-20 12:04:34Z

The problem is the order of the prep() function. If you change to push an element into the Element object, and then push it tho the entries vector, the behavior will be the expected.

    void prep()
    {
        Entry* entry = new Entry();
        entry->list.push_back(0);
        entries.push_back(*entry);
        cout << "prep " << entry->list.size() << "\n";
    }

This is happening, because you uses a copy in the entries list.

It is also possible to store the pointer of the object therefore you can edit the actual instance after you pushed to the entries vector.

Edit:

As Ted mentioned, there is a memory leak, because the entry created with the new operator never deleted. Another approach could be to use smart pointers (however, in this small example it seems overkill, just use reference)

    void prep()
    {
        std::unique_ptr<Entry> entry = std::make_unique<Entry>();
        entry->list.push_back(0);
        entries.push_back(*entry.get());    // get the pointer behind unique_ptr, then dereference it
        cout << "prep " << entry->list.size() << "\n";
    }    // unique_ptr freed when gets out of scope

Marius Bancila · Accepted Answer · 2021-04-20 11:14:16Z

1

You need to change the implementation of prep():

void prep()
{
    Entry entry;
    entry.list.push_back(0);
    entries.emplace_back(entry);
    cout << "prep " << entries.back().list.size() << "\n";
}

There is no need to allocate a Entry on the heap just to make a copy of it.

answered Apr 20, 2021 at 11:14

Marius Bancila

16.4k9 gold badges53 silver badges97 bronze badges

2 Comments

pid Over a year ago

Thank you for your answer! But I have to accept the other one because @ted was slightly faster and I had already fixed the code when I came back here and read yours. Thanks again and have a nice day!

Marius Bancila Over a year ago

No worries, what's important to have your problem fixed.

Collectives™ on Stack Overflow

Add an element to a vector inside a struct, from within another struct

3 Answers 3

2 Comments

Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related