Trying to learn boost::intrusive Q3 - When storing pointers in ICs, should I use smart_pointer?

Question

I have progressed greatly in my understanding of intrusive containers. I have a program that runs for a "while", and then on a line of code like this delete *it; (see below):

....

                // :  public list_base_hook< void_pointer< ip::offset_ptr<void> > >
class OneDepthPrice : public list_base_hook<link_mode<auto_unlink>> // This is a derivation hook
{
public:
    Provider provider;
    Price price;
public:
    list_member_hook<link_mode<auto_unlink>> member_hook_; // This is a member hook

    OneDepthPrice(Provider prov, Price p) : provider(prov), price(p) {}
};

...

std::vector<OneDepthPrice *>& vecPrices

for (auto it = vecPrices.begin(); it != vecPrices.end();  ++it)
{
    auto& e = *it;
#if DEBUG
    std::cout << e->provider.name << "\n" << std::flush;
#endif
    if(e->provider.name == newPrices.provider.name)
    {
        delete *it; //This is the offending line in the stack trace in the debugger.

        it = vecPrices.erase(it);
    }
}

the program crashes with this stack trace:

#0 0x407ddd boost::intrusive::list_node_traits<void*>::set_next(n=@0x7fffffffe2b8: 0x0, 
next=@0x7fffffffe2b0: 0x706860) (/usr/local/include/boost/intrusive/detail/list_node.hpp:64)
#1 0x409189 boost::intrusive::circular_list_algorithms<boost::intrusive::list_node_traits<void*> >::unlink(this_node=@0x7fffffffe2e8: 0x706830) (/usr/local/include/boost/intrusive/circular_list_algorithms.hpp:140)
#2 0x407e2a boost::intrusive::generic_hook<boost::intrusive::get_list_node_algo<void*>, boost::intrusive::default_tag, (boost::intrusive::link_mode_type)2, (boost::intrusive::base_hook_type)1>::unlink(this=0x706830) (/usr/local/include/boost/intrusive/detail/generic_hook.hpp:180)
#3 0x406b5c boost::intrusive::detail::destructor_impl<boost::intrusive::generic_hook<boost::intrusive::get_list_node_algo<void*>, boost::intrusive::default_tag, (boost::intrusive::link_mode_type)2, (boost::intrusive::base_hook_type)1> >(hook=...) (/usr/local/include/boost/intrusive/detail/utilities.hpp:371)
#4 0x405b13 boost::intrusive::generic_hook<boost::intrusive::get_list_node_algo<void*>, boost::intrusive::default_tag, (boost::intrusive::link_mode_type)2, (boost::intrusive::base_hook_type)1>::~generic_hook(this=0x706830, __in_chrg=<optimized out>) (/usr/local/include/boost/intrusive/detail/generic_hook.hpp:160)
#5 0x40534a boost::intrusive::list_base_hook<boost::intrusive::link_mode<(boost::intrusive::link_mode_type)2>, void, void>::~list_base_hook(this=0x706830, __in_chrg=<optimized out>) (/usr/local/include/boost/intrusive/list_hook.hpp:86)
#6 0x405546 OneDepthPrice::~OneDepthPrice(this=0x706830, __in_chrg=<optimized out>) (/home/idf/Documents/TestCPPArrays/TestCPPArrays.cpp:86)
#7 0x403728 UpdateBunchTogether(vectogether=..., vecPrices=..., newPrices=...) (/home/idf/Documents/TestCPPArrays/TestCPPArrays.cpp:291)
#8 0x404765 main() (/home/idf/Documents/TestCPPArrays/TestCPPArrays.cpp:558)

This is a strange bug because the program is not multi-threaded but it runs for a while without a hitch. I am not sure what is happening, but maybe I need to use smart_pointers?

Question: why are you using intrusive containers? If you're storing pointers, then by all means, use regular containers. — sehe, Nov 11 '14 at 23:48
sehe, the biggest reason is that I like ICs, i.e., the semantics of not having to worry about all the different places that an object is stored/referenced. I simply delete it from one place, and all the other places are automatically updated. — Ivan, Nov 11 '14 at 23:50
I will say this though, I may very well be able to use regular objects and not pointers. I just can't see to get that to work. — Ivan, Nov 11 '14 at 23:52
Then you indeed want shared_pointers, or flyweights (with tracking enabled). You do not want Intrusive Containers just for this, because you get into the mess of lifetime management. — sehe, Nov 11 '14 at 23:52
Ok, I was led to this solution by your suggestion in ICs Q1. I also see that they are very high performing which is extremely desirable. I will look into your other suggestions. Do you mean sp+ics? sps don't have ic neat feature that I only have to worry about one place of removing them? Don't know what a flyweight is... — Ivan, Nov 11 '14 at 23:58
[Probably] unrelated issue - you do `it = vecPrices.erase(it);` correctly, but then you unconditionally do `++it;`. You want to only increment `it` if you didn't erase... otherwise you could walk past the end of your vector. — Barry, Nov 11 '14 at 23:58
Don't roll answers into question, especially not directly. The answer can be edited for clarity for your original problem. — Yakk - Adam Nevraumont, Nov 12 '14 at 00:29
Okay, I've just completed a sample that might inspire you using weak_ptrs. See this recent sample for an applying refcounted Boost Flyweight: ***[in this comment](http://stackoverflow.com/questions/26825538/lookup-on-nested-boost-multi-index-container/26832795#comment42245382_26832795)*** — sehe, Nov 12 '14 at 00:38
Ironically, my answer [here](http://stackoverflow.com/a/26876194/85371) already contained the correct code to do the predicated erasing iterator loop :) — sehe, Nov 12 '14 at 00:40

score 2 · Accepted Answer · answered Nov 12 '14 at 00:06

Moving my comment to an answer. When you do:

for (auto it = vecPrices.begin(); it != vecPrices.end();  ++it)
{
    auto& e = *it;
    if(e->provider.name == newPrices.provider.name)
    {
        delete *it; //This is the offending line in the stack trace in the debugger.

        it = vecPrices.erase(it);
    }
}

You correctly update the iterator when you erase, but then you unconditionally increment it. This is bad for two reasons: you could fail to delete the next object if you need to, and if erase() returns end() then you just walked past the end of your vector.

To erase safely, you need to do:

for (auto it = vecPrices.begin(); it != vecPrices.end();  /* nothing */)
{
    auto& e = *it;
    if(e->provider.name == newPrices.provider.name)
    {
        delete *it; //This is the offending line in the stack trace in the debugger.

        it = vecPrices.erase(it);
    }
    else 
    {
        ++it; // this is where we increment
    }
}

@ivan I would write a custom `erase_remove_if` that operates on a container rather than maintain the above mess of looping and business logic. — Yakk - Adam Nevraumont, Nov 12 '14 at 00:26

sehe · Answer 2 · 2014-11-12T00:31:30.293

Actually with Boost Intrusive you worry about where an object is referenced all the time (because references are all over the place).

Maybe you just want weak_ptr. It also looks as though you're trying to implement garbage collection. Here's a slightly reworked idea using weak_ptr:

Live On Coliru

#include <vector>
#include <map>
#include <boost/weak_ptr.hpp>
#include <boost/make_shared.hpp>
#include <iostream>

using std::string;
using boost::shared_ptr;
using boost::weak_ptr;
using boost::make_shared;

struct Person {
    string name_;
    Person(string name) : name_(move(name)) {}

    using tag = weak_ptr<void>;

    tag track() const { return tracking_tag; }

  private:
    shared_ptr<void> tracking_tag = make_shared<char>('x');
};

template <typename T>
inline bool IsDeleted(weak_ptr<T> const& v) {
    return !v.lock();
}

enum favcolor { red, blue, pink, magenta, beige } favorite_color;

template <typename Map> size_t garbage_collect(Map& map)
{
    size_t collected = 0;
    for (auto it = map.begin(); it!=map.end();)
    {
        if (IsDeleted(it->first))
            ++collected, it = map.erase(it);
        else
            ++it;
    }

    return collected;
}

int main()
{
    std::vector<Person> people;

    for (auto&& name : { "John", "Mike", "Garbarek", "Milou", "Confucius", "Kiplat" })
        people.emplace_back(name);

    struct Properties {
        favcolor favorite_color;
        struct Car { string brand; int year; } vehicle;
    };

    std::map<Person::tag, Properties> associated {
        { people[0].track(), Properties { magenta, { "Chevy", 1986 } } },
        { people[2].track(), Properties { pink,    { "Kia",   2011 } } },
    };

    for (auto& p : people) std::cout << p.name_ << " "; std::cout << "\n";
    std::cout << "Defined properties: " << associated.size() << "\n";

    people.erase(std::remove_if(people.begin(), people.end(), [](Person const& p) { return p.name_[1] == 'o'; }), people.end());

    for (auto& p : people) std::cout << p.name_ << " "; std::cout << "\n";
    std::cout << "Defined properties before garbage collect: " << associated.size() << "\n";

    auto count = garbage_collect(associated);
    std::cout << "Defined properties after garbage collect: " << associated.size() << " (" << count << " collected)\n";
}

Output:

John Mike Garbarek Milou Confucius Kiplat 
Defined properties: 2
Mike Garbarek Milou Kiplat 
Defined properties before garbage collect: 2
Defined properties after garbage collect: 1 (1 collected)

Trying to learn boost::intrusive Q3 - When storing pointers in ICs, should I use smart_pointer?

2 Answers2