c++, I got a function that returns a new pointer, is this bad?
Author
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
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.
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()
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;
}
for(;;) {
triangles* = getTriangles(/*should pass parameters*/);
.
do some stuff...
.
delete [] triangles;
}
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.
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.
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.
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.
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.
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.
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 workThanks for the info.
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.)
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...
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...
Author
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:
I dont understand the reason to return a pointer here instead of just the vector<Tria...>?
I don't understand, do you mean have the function return one triangle at a time?
Hmm i could do that actually....
edit: like this?
Thanks
...Too bad 'auto_ptr' doesn't work with arrays ;(
[Edited by - johnnyBravo on June 24, 2005 1:06:29 AM]
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 = 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]
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
