Destroying set of smart pointers segfaults

[Matthias Kaeppler]

Hi,

I thought it'd be a better idea to start a new thread dealing directly
with my problem. Okay, here's again what's happening:
I'm storing boost::shared_ptrS in an std::set. I supposed that on
program exit, when the set's dtor is called, the shared_ptrS would clean
up after themselves appropriately. What actually happens though is that
the program segfaults when the set is destructed.

I noticed that erasing the smart pointers from the std::set does not
work as expected. IIRC, std::set finds its elements by checking for
/equivalance/, right? That means, it would check for !less && !greater
right?
So, does that work with boost::shared_ptr at all? I know it features an
operator==, but that would only work if the container would check for
equality, which is not the case.

But then, even if my observation would be correct, I can't explain why
destroying the set would cause the program to segfault.

Any ideas?

Thanks,
Matthias

 

[Matthias Kaeppler]

Here are the relevant parts of my program code:

class Task {/* ... */}; // the definition is not relevant

// this class stores the set in question
class TaskManager
{
public:
void add_task(Task*);
void remove_task(Task*);
// ...
private:
std::set< boost::shared_ptr<Task> > tasks_;
// ...
};

void TaskManager::add_task(Task *task)
{
tasks_.insert(boost::shared_ptr<Task>(task));
// ...
task->start();
}

void TaskManager::remove_task(Task *task)
{
task->stop();
// ...
tasks_.erase(boost::shared_ptr<Task>(task)); // this does in fact
NOT erase anything
}

Tasks are added by calling TaskManager::add_task():

// in some other class:
taskmanager.add_task(new Task(...));

Hope that helps.
- Matthias

 

[Max M.]

tasks_.erase(boost::shared_ptr<Task>(task)); // this does in fact

You can't legally initialize a new shared_ptr with a given pointer, if
there are other shared_ptr's around pointing to the same object. You
probably could, if an intrusive reference counting technique were used,
which is not the case.

You should probably handle every 'task' object using a shared_ptr as it
gets instantiated, and change TaskManager's interface so that it accepts
shared_ptr arguments.

Max

 

[David Hilsee]

[...]

tasks_.erase(boost::shared_ptr<Task>(task)); // this does in fact
NOT erase anything
}

[...]

As it shouldn't. From the docs:

 

[Pete Becker]

Here are the relevant parts of my program code:

class Task {/* ... */}; // the definition is not relevant

// this class stores the set in question
class TaskManager
{
public:
void add_task(Task*);
void remove_task(Task*);
// ...
private:
std::set< boost::shared_ptr<Task> > tasks_;
// ...
};

This is getting you in trouble. You can't mix raw pointers and
shared_ptr. If you're going to use a shared_ptr to control a resource
you must initialize a shared_ptr object with the pointer to the resource
and then abandon the pointer. (It's not quite that drastic, but it's a
good rule of thumb when you're getting started):

shared_ptr<Task> sp(new Task);

And now your TaskManager's interface should traffic in shared_ptr<Task>s
rather than Task*'s.

 

[Matthias Kaeppler]

Okay, thanks for all your input.

One more question:
When passing around a shared_ptr, is it better practice to do it by
reference or by value?

e.g.:
void foo(const shared_ptr<Bar>& ptr);
or
void foo(shared_ptr<Bar> ptr);

I mean, constructing a copy is certainly not that expensive I guess, but
if it's reference counted, it would imply a useless increment and
decrement of the reference counter right?
Right now, I pass it by reference to const, just to be safe, although
this looks kind of weird for a "pointer" (I usually try to keep my code
intuitive, and this would imply to pass it by value just like a raw
pointer).

Regards,
Matthias