Sign in to follow this  

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

This topic is 4559 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

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

This topic is 4559 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.

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