Sign in to follow this  
mastodon

Question about using boost::shared_ptr in an std::list or vector

Recommended Posts

I'm writing a scene manager which has an std::list of all of the nodes. Here is some code:
std::list< boost::shared_ptr<SceneNode> > _nodes; // List of all scene nodes.

StaticMeshNode* SceneManager::addStaticMeshNode(const char* filename) {
        boost::shared_ptr<StaticMeshNode> node(new StaticMeshNode());
	node.get()->initialize(filename, _resourceManager, _renderer, this);
	_nodes.push_back(node);

	return node.get();
}

My code compiles and seems to work fine, but I am just a little suspicious that I might be doing something a bit off. I've been reading the boost documentation but I just can't seem to apply it to how it is used in a list or a vector. Any comments/help would be helpful. Thanks

Share this post


Link to post
Share on other sites
Nope, that'll work fine, and will do just what you expect. One thing, though... what's up with having an empty constructor and an "initialize" member? When you do that sort of thing, you make Bjarne Stroustrup cry.

Share this post


Link to post
Share on other sites
I was actually noticing the same thing while I was implementing the shared_ptrs. I think it was because each scene node needs different parameters to initialize. I realize that isn't necessary now but when I wrote this (a couple years ago) I think that's what I had in mind.

Share this post


Link to post
Share on other sites
A couple of additional comments:

std::list< boost::shared_ptr<SceneNode> > _nodes; // List of all scene nodes.

StaticMeshNode* SceneManager::addStaticMeshNode(const char* filename)
{
boost::shared_ptr<StaticMeshNode> node(new StaticMeshNode());

// Most smart pointers (and this includes the Boost smart pointers)
// are implemented so that they mimic the semantics of 'raw' pointers.
// As such, you can use the * and -> operators directly; you don't
// need to use the get() function here:
node/*.get()*/->initialize(filename, _resourceManager, _renderer, this);
_nodes.push_back(node);

// Note that if you follow Sneftel's suggestion (which you should),
// the above becomes:

/*
_nodes.push_back(boost::shared_ptr<StaticMeshNode>(
filename, _resourceManager, _renderer, this));
*/


// Note that you can clean up the code a bit by using a typedef.

// Finally, I'm not sure what you're doing here, but generally speaking,
// when you invoke the 'get' function, you defeat the purpose of using
// a smart pointer. Here, it looks like you're sharing ownership of
// the object by way of a raw pointer, which completely undermines the
// purpose of the shared_ptr class (which is to manage shared ownership
// by way of reference counting).

// The typical thing to do here would be to return a shared_ptr object.
return node.get();
}

Share this post


Link to post
Share on other sites
Quote:
Original post by SiCrane
Also, if you're using smart pointers, why return a dumb pointer from the function?



How exactly would I do that? Pass _nodes.back() by reference?

I think of the shared_ptr as a structure containing a pointer. So returning a shared_ptr itself would be returning a copy? Am I thinking about this incorrectly?

Share this post


Link to post
Share on other sites
Quote:
Original post by jyk
A couple of additional comments:

*** Source Snippet Removed ***


Doesn't this break one of the rules in the boost documentation?


/* Avoid using unnamed shared_ptr temporaries to save typing; to see why this is dangerous, consider this example:*/

void f(shared_ptr<int>, int);
int g();

void ok()
{
shared_ptr<int> p(new int(2));
f(p, g());
}

void bad()
{
f(shared_ptr<int>(new int(2)), g());
}




Share this post


Link to post
Share on other sites
Quote:
Original post by mastodon
How exactly would I do that? Pass _nodes.back() by reference?


You wouldn't. You pass it back by value.

Quote:
I think of the shared_ptr as a structure containing a pointer. So returning a shared_ptr itself would be returning a copy? Am I thinking about this incorrectly?


It would return a copy of the shared_ptr<>, but the underlying object pointed at is not copied.

Try this experiment:


#include <boost/shared_ptr.hpp>
#include <iostream>

int main()
{
using namespace boost;

shared_ptr<int> p1(new int(5));
shared_ptr<int> p2(p1); // copy

std::cout << p1.get() == p2.get() << '\n';

return 0;
}



Share this post


Link to post
Share on other sites
Quote:
Original post by mastodon
Quote:
Original post by jyk
A couple of additional comments:

*** Source Snippet Removed ***


Doesn't this break one of the rules in the boost documentation?

*** Source Snippet Removed ***


jyk's code is perfectly safe. The point of the example in the boost documentation relates to exception safety.

Consider this call:


f(shared_ptr<int>(new int(2)), g());



Here, the compiler may do things in this order:


int *temp1 = new int(2);
int temp2 = g();
shared_ptr<int> temp3(temp1);
f(temp3, temp2);



If g() throws an exception, you have a memory leak because temp1 never gets deleted.

By creating a shared_ptr<> separately, we avoid this situation:


shared_ptr<int> p(new int(2));
f(p, g());



Now there's no way that a memory leak will occur, even if g() throws an exception.

Share this post


Link to post
Share on other sites
Quote:
Original post by mastodon
Quote:
Original post by jyk
A couple of additional comments:

*** Source Snippet Removed ***


Doesn't this break one of the rules in the boost documentation?

*** Source Snippet Removed ***


I think the point was that the initialize method should really be a constructor and not to collapse it all into a temporary like the boost docs say not to do.
e.g.:

typedef boost::shared_ptr<StaticMeshNode> NodePtr;

NodePtr SceneManager::addStaticMeshNode(const char* filename) {
NodePtr node(new StaticMeshNode(filename, _resourceManager, _renderer, this));
_nodes.push_back(node);

return node;
}


Share this post


Link to post
Share on other sites
Mastadon, regarding returning dumb/smart pointers and using shared_ptr in a container:

You should think about object ownership. If a SceneNode is allowed exist without being in the _nodes collection (BTW don't use _variable in your code, leading underscores are reserved for system stuff), then you should use a list or vector of shared_ptrs, and allow access through shared_ptrs (or weak_ptr's, but that's another discussion).

However, if _nodes owns all SceneNodes (i.e. is respoinsible for creating/deleting them) then you might find it easier to use a boost pointer container. This approach more correctly models exclusive ownership and as a bonus is marginally faster as well.

Share this post


Link to post
Share on other sites
Quote:
Original post by ChaosEngine
Mastadon, regarding returning dumb/smart pointers and using shared_ptr in a container:

You should think about object ownership. If a SceneNode is allowed exist without being in the _nodes collection (BTW don't use _variable in your code, leading underscores are reserved for system stuff), then you should use a list or vector of shared_ptrs, and allow access through shared_ptrs (or weak_ptr's, but that's another discussion).

However, if _nodes owns all SceneNodes (i.e. is respoinsible for creating/deleting them) then you might find it easier to use a boost pointer container. This approach more correctly models exclusive ownership and as a bonus is marginally faster as well.


Well... A SceneNode really has no reason to exist if the SceneManager does not exist. I was using shared_ptr only for assuring that all nodes would eventually be deleted..

Using a boost::ptr_vector might be what I'm looking for. Would I just return standard pointers in my adding functions?

Share this post


Link to post
Share on other sites
Quote:
Original post by mastodon
Quote:
Original post by ChaosEngine
Mastadon, regarding returning dumb/smart pointers and using shared_ptr in a container:

You should think about object ownership. If a SceneNode is allowed exist without being in the _nodes collection (BTW don't use _variable in your code, leading underscores are reserved for system stuff), then you should use a list or vector of shared_ptrs, and allow access through shared_ptrs (or weak_ptr's, but that's another discussion).

However, if _nodes owns all SceneNodes (i.e. is respoinsible for creating/deleting them) then you might find it easier to use a boost pointer container. This approach more correctly models exclusive ownership and as a bonus is marginally faster as well.


Well... A SceneNode really has no reason to exist if the SceneManager does not exist. I was using shared_ptr only for assuring that all nodes would eventually be deleted..

Using a boost::ptr_vector might be what I'm looking for. Would I just return standard pointers in my adding functions?
I've never used the Boost pointer containers, but my understanding is that they assume exclusive ownership of the passed-in pointers. If that is in fact the case, then I doubt you'd want to go passing the pointers around. (If that's what you had in mind, I'd just stick with shared_ptr.)

Share this post


Link to post
Share on other sites
Quote:
Original post by jyk
I've never used the Boost pointer containers, but my understanding is that they assume exclusive ownership of the passed-in pointers. If that is in fact the case, then I doubt you'd want to go passing the pointers around. (If that's what you had in mind, I'd just stick with shared_ptr.)


Depends on what you mean by "passing the pointers around". Of course, storing a copy of the exclusively owned pointer is not necessarily clever, but there's no reason you can't do something like:


for (boost::ptr_vector<SceneNode>::const_iterator iter = _nodes.begin();
iter != _nodes.end(); ++iter)
{
Render(*iter);
}


or something similar.

OTOH, this


SceneNode* GetNode(int index)
{
boost::ptr_vector<SceneNode> nodes;
nodes.push_back(new SceneNode);
nodes.push_back(new SceneNode);
nodes.push_back(new SceneNode);


return nodes[index];
}
is definitely not smart! [grin]

Share this post


Link to post
Share on other sites
Quote:
Original post by ChaosEngine


SceneNode* GetNode(int index)
{
boost::ptr_vector<SceneNode> nodes;
nodes.push_back(new SceneNode);
nodes.push_back(new SceneNode);
nodes.push_back(new SceneNode);


return nodes[index];
}
is definitely not smart! [grin]
Yes, that's the sort of thing I had in mind :)

Actually though, I believe it would be 'return &nodes[index]', since in general the Boost pointer containers return references, not pointers (IINM).

Share this post


Link to post
Share on other sites
Quote:
Original post by jyk
I've never used the Boost pointer containers, but my understanding is that they assume exclusive ownership of the passed-in pointers.


That is my understanding also.

Quote:
If that is in fact the case, then I doubt you'd want to go passing the pointers around. (If that's what you had in mind, I'd just stick with shared_ptr.)


Would you feel safer about passing around weak_ptrs constructed from the container contents? Or perhaps references? (After all, ptr_foo<T>::value_type is T rather than T*.)

As for the "dangerous temporary": jyk's code should be fine, but making the shared_ptr variable explicit (a) is definitely fine and (b) avoids making a .back() call for something we just inserted.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this