Jump to content
  • Advertisement
Sign in to follow this  
mastodon

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

This topic is 3797 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

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
Advertisement
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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!