Sign in to follow this  
johnnyBravo

c++, I got a function that returns a new pointer, is this bad?

Recommended Posts

Hi, I've got a function that returns a 'new' pointer. eg //this function has been cut down, to just show what i mean. Triangle* getTriangles(Object* obj) { Triangle* t = new Triangle[verticesCount/3]; return t; } So when using it, I would say Triangle* triangles = getTriangles(); //later delete [] triangles Is this bad design? The reason I want to do this is because I want that function to return the triangles that have been orientated to the object's orientation that is calling that function. So I can just get the data i need from the function. And is there a way to make some kind of pointer or class that will delete itself when it goes out of scope. eg like a vector<Triangle> ? Thanks

Share this post


Link to post
Share on other sites
Well... some people might call that "bad design"... but it's not necessarily "bad"... but memory leaks are a possibility.

What it sounds like you need is a smart pointer of some type to hold your array; it would get deleted automagically once the smart pointer goes out of scope. Ive never tried anything like

boost::shared_ptr<ClassType[]>

or

Loki::SmartPtr<ClassType[]>

...so maybe you could just return a pointer to a new "vector<Triangle>" instead of a pointer to an array.

std::vector<Triangle> *getTriangles()

And you *can* use boost's smart pointers and Loki's smart pointers with vectors.


boost::shared_ptr<std::vector<Triangle> > getTriangles()

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
well, it's ok if you're going to 'delete []' as soon as you're finished using the triangle, becuase every call to this function will allocate another 'Traingle' so let's say:
for(;;) {
triangles* = getTriangles(/*should pass parameters*/);
.
do some stuff...
.
delete [] triangles;
}

Share this post


Link to post
Share on other sites
Yep. There's auto_ptr (also read this for some good practices) from the Standard Template Library, and also shared_ptr, smart_ptr, etc. from the Boost libraries.

Naked pointers can be used intelligently, but we all make mistakes sometimes, even if it's just a "I'll figure out where to put that delete call later" thought which turns out to never happen. There's a little extra typing to put smart pointers into your code, and some knowledge of how they work to use them effectively, but it pays off a lot in the end.

It's good that you've considered this before making silly mistakes later on that cause all kinds of hair pulling and headaches.

EDIT:
Ah! just realized that you are probably creating an array of triangles... in which case a simple auto_ptr would only delete the first element (or something otherwise hideous, but not delete [] 'ing the whole array).

What you want to use is the scoped_array from Boost. Or, you could use shared_array if the pointer will be reused or copied by other parts of your code... look at the entire smart_ptr library for more details.

Share this post


Link to post
Share on other sites
Just make sure you're not storing your naked "Triangle *" pointer in a smart pointer, because you're allocating an array... and the smart pointer would call "operator delete" instead of "operator delete[]".

It may be best to create a new vector of Triangles and return THAT wrapped in a smart pointer.

void * gave you boost's web address... check Loki as well for more smart ptr options.

Share this post


Link to post
Share on other sites
What you have there - returning freshly allocated memory - is considered "evil". You should only ever do it when you know exactly what you are doing (and then - you should document it very well with comments).

In your situation - returning that freshly allocated memory is a bad idea. It's just a bunch of triangles after all. There is no need for the added complexity.

You have several options - including returning a std::vector<Triangle> or using a smart pointer of some description (I suggest an auto pointer or scoped pointer, but a shared pointer would work too). Remember your smart pointer must be of the array variety (what Verg has posted [EDIT: in his first post - not the one directly above] is incorrect - you can't use boost::shared_ptr for an array - use boost::shared_array).


One of the main reasons this is particularly bad is that it breaks RAII and makes the calling function exception-unsafe (thus it must go to measures to ensure exception safety - like dropping it in a smart pointer anyway). For instance - if an exception is thrown after you get your triangles but before you delete them, you'll leak that memory. Even if you don't use exceptions - it's good form to write exception-safe code.

Share this post


Link to post
Share on other sites
Quote:
Original post by Andrew Russell
what Verg has posted [EDIT: in his first post - not the one directly above] is incorrect - you can't use boost::shared_ptr for an array


Yeah... see, I'd never tried that... so it wasn't recommended [cool]


boost::shared_ptr<ClassType[]> ptr; // doesn't work


Thanks for the info.

Share this post


Link to post
Share on other sites
You should be creating an object that owns the triangle list, and returning a pointer to it.

Something like IMesh* CreateMesh(...)

And the concrete mesh class can manage its vertex buffer(s).

IMesh should be integrated in your object-lifetime-management strategy.
You could just return a raw pointer, you could change it to auto_ptr, you could use reference counting, etc...

Share this post


Link to post
Share on other sites
Hmmm i'd rather not use external libraries. eg boost, loki etc. As I don't want to have extra libraries lying around.
Although boost and loki do look like good ideas...

Would vector be ok speed wise? eg I might have say on average 4 triangles in the list, and there might be 400 or so calls to that function per loop.
eg if i were to return <Triangle>,

also with:
Quote:

...so maybe you could just return a pointer to a new "vector<Triangle>" instead of a pointer to an array.

std::vector<Triangle> *getTriangles()

I dont understand the reason to return a pointer here instead of just the vector<Tria...>?



Quote:

Or, you could pass in an array and fill in each triangle (make sure to pass in an integer telling how many triangles in the array.)


I don't understand, do you mean have the function return one triangle at a time?

Quote:

You should be creating an object that owns the triangle list, and returning a pointer to it.


Hmm i could do that actually....
edit: like this?

class TriangleList {
private:
Triangle *triangles;
int trianglesCount;
public:
//the getTriangles(..) function's class converts a point list to triangles...
TriangleList(Vector3 *points, int trianglesCount) {
this->trianglesCount = trianglesCount;
triangles = new Triangle[triangleCount];

for(int i=0;i<triangleCount;i++) {
triangles[i] = Triangle(points[i*3],points[i*3+1],points[i*3+2]);
}
}
~TriangleList() {
delete [] triangles;
}
int getTrianglesCount() {
return trianglesCount;
}
const Triangle *getTriangles() {
return triangles;
}
};




Thanks

...Too bad 'auto_ptr' doesn't work with arrays ;(

[Edited by - johnnyBravo on June 24, 2005 1:06:29 AM]

Share this post


Link to post
Share on other sites
Returning a raw pointer is usually a bad idea because it's unclear who has responsibility for free the allocated memory (or even how to free it - delete, delete[], some sort of custom allocator).

It can be done safely as a private function for class implementation, seeing as the class it belongs to can take responsiblity for deallocation.

Share this post


Link to post
Share on other sites
Personally I see nothing wrong in functions that return newly allocated objects, as long as they're clearly marked as such (typically with a large comment saying /* FACTORY METHOD */ or similar. But of course, as others point out, if you can return it in a safer form without compromising your design or performance, do that instead.

Share this post


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

Or, you could pass in an array and fill in each triangle (make sure to pass in an integer telling how many triangles in the array.)


I don't understand, do you mean have the function return one triangle at a time?


Triangle* Array = new Triangles[NUMBER_THAT_IS_NEEDED];

void SomeFunction(Array, NUMBER_THAT_IS_NEEDED);

delete [] Array;

Share this post


Link to post
Share on other sites
Quote:
Original post by Kylotan
Personally I see nothing wrong in functions that return newly allocated objects, as long as they're clearly marked as such (typically with a large comment saying /* FACTORY METHOD */ or similar. But of course, as others point out, if you can return it in a safer form without compromising your design or performance, do that instead.


*Nods* look up the Factory Pattern

If your function contract is clear, then there's no harm in returning a new object.

Share this post


Link to post
Share on other sites
In those kind of cases, I usually go for this:



void func()
{
int elementCount;
TElement *element;

elementCount=Object.GetElementCount();//get number of elements
element=new TElement[elementCount];//alloc memory
Object.GetElements(element);//fill in the array of elements

...//Use elements here...

delete[] element;
}



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