• Advertisement
Sign in to follow this  

calling destructor versus new object

This topic is 1629 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,

Just wondering, when I run my d3d engine main program, I have 1 instance of the class d3dscene.

When I go to another scene, I call the destructor and load the new one.

This keeps my main loop clean, always rendering the same d3dscene object.

 

My question;

- what would be 'better', create a regular 'object' on the stack, i.e. "engine::d3dscene myscene", when finished manually call the destructor + constructor and then call it's member function 'loadscene'?

 

versus

 

- engine::d3dscene *myscene = new engine::d3dscene;

and when finished, delete myscene and again "*myscene = new engine::d3dscene

 

In the 1st option calling the constructor is necessary to reset all counters, vars etc., and it's working fine.

 

What would you say?

Edited by cozzie

Share this post


Link to post
Share on other sites
Advertisement
I would make an instance part of my scene object, and then the constructors and destructors will get called at the right times without doing anything special.

EDIT: Oh, but then you might have the same question about your scene object, so I probably didn't really answer the question. If scenes can be assigned, you might be able to do the clean-looking thing using assignment of objects. Edited by Álvaro

Share this post


Link to post
Share on other sites

Hi alvaro.

Thanks, not sure if I understand it though.

 

I'm talking about the main objects of my application (containing mesh instances, audio, etc.).

So basically:

Engine_d3drenderer::	CD3d		_d3d;
Engine_d3drenderer::	CD3dcam		_d3dcam(D3DXVECTOR3(0.0f, 0.0f, 0.0f)); 
Engine_d3drenderer::	CD3dscene	_d3dscene;

Engine_dxinput::		CDinput		_dinput; 
Engine_audio::			CAudio		_audio;

Engine_IO::				CScene		_scene;
Engine_general::		CTimer		_timer;
Engine_game::			CPlayer		_player(Crealysm_math::VECTOR3(0.0f, 0.0f, 0.0f));

Where _scene, _d3dscene and _audio will be 'renewed' when I load another scene.

What I do know is:

// destruct the IO Scene, D3D scene and camera object
_scene.~CScene();
_d3dscene.~CD3dscene();
_d3dcam.~CD3dcam();

And then just call the member functions for loading the new scene etc.

 

My question is if it would be better do make a pointer to the object for those 3 'varying' objects:

Engine_d3drenderer::	CD3dcam		*_d3dcam = new Engine_IO::d3drenderer::CD3dcam(D3DXVECTOR3(0.0f, 0.0f, 0.0f)); 
Engine_d3drenderer::	CD3dscene	*_d3dscene = new Engine_IO::d3drenderer::CD3dscene;
Engine_IO::		CScene		*_scene = new Engine_IO::CScene;

And then when loading a new scene:

_scene = new Engine_IO::CScene;
_d3dscene = new Engine_d3drenderer::CD3dscene;
_d3dcam = new Engine_d3drenderer::CD3dcam;

Where to 2nd option prevents me from calling both the destructor and constructor manually I think.

This would also prevent me from throwing around the whole object in functions, instead of just a a pointer.

Share this post


Link to post
Share on other sites

I find the manual destructor calling highly suspicious. It's an obscure c++ feature that you should only use if you know 100% what you are doing and isn't needed for regular day to day programming at all. Why don't you delete your objects and then new them?

 

Edit:

 

I should try to read better. The first part still stands though.

Waterlimon's suggestion is good. Alternatively reset all the variables in the loadscene method. After all, you could call the loadscene method more than once, so it should leave the scene in a clean state.

Edited by Madhed

Share this post


Link to post
Share on other sites

Hey, you can't call a constructor.  You can construct an object (in which case the constructor gets invoked), but the C++ language provides no way to call a constructor directly.

 

If you're doing what I think you're doing, you're probably invoking the destructor directly (which is, technically, undefined behaviour for objects not created using placement new) and then calling an initializer function to reset the object values.  The proper way to do that in C++ is to use the assignment operator to assign a new object value to the existing object instance.

 

For example:

Engine_d3drenderer::CD3dscene    _d3dscene;
// ... use the scene
_d3dscene = Engine_d3drenderer::CD3dscene(); // destroy the old object and assign a new, freshly initialized value

Share this post


Link to post
Share on other sites

Placement new is effectively a direct call to the constructor. You can also call it directly like any other member function. See http://www.gamedev.net/page/resources/_/technical/general-programming/wade-not-in-unknown-waters-part-one-r3290 for examples.

 

My suggestion would be to use new and delete as normal. In general when in doubt, you should probably go for whatever solution will be simplest to understand. Being easy to understand means it should also be easier to debug and change later if you need to do so.

 

 

 

Edit: You can't actually call the constructor directly like a normal function. I still say placement new does the same thing as calling the constructor would, even for non-POD classes. That's what it's there for. http://www.drdobbs.com/cpp/calling-constructors-with-placement-new/232901023?pgno=2

Edited by Adam_42

Share this post


Link to post
Share on other sites


- what would be 'better', create a regular 'object' on the stack, i.e. "engine::d3dscene myscene", when finished manually call the destructor + constructor and then call it's member function 'loadscene'?
Better according to what? Sure not about perf: with 1 creation every... couple of minutes?

Anyway, when putting stuff on stack, don't force your rules. Just let normal scope rules take care of that. Put it inside a loop, a brace block an if, what fits your liking.

FYI, my current scene manager is manually managed through new/delete, being part of a much higher-level, longer-living object. Never even spotted it on the profiler.

Architecturally speaking, I'd prefer to avoid using new/delete whatever possible but for this specific example I think there's just too little to gain.

Share this post


Link to post
Share on other sites

Placement new is effectively a direct call to the constructor. You can also call it directly like any other member function.

No it's not and no you can't.

When an object is constructed, even using placement new, more than just the constructor body is invoked. Yes, if you have a simple class without any kind of inheritance in which all its members are PODs and there is no initializer list, and you aren't using exceptions or RTTI anywhere in your application, placement new is effectively the same as calling the constructor like a function but using a weird syntax. That's a pretty broad range of restrictions to say placement new is effectively a direct call to the constructor. It's more accurate to say "constructing and object using placement new is a way of constructing an object, and one of the things that happens when an object is constructed is that the constructor is invoked."

A conformant compiler will not allow a constructor to be invoked directly since it does not have a name and does not participate in name lookup (see [12.1](2) of the standard) -- it should result in a compile-time error.

Share this post


Link to post
Share on other sites

Thanks all.

I'm now trying to initialize the scene as a fresh object, like Bregma suggested and showed in his example.

 

Unfortunately no luck yet, because that line "CScene = engine::IO:CScene()" is giving an access violation error.

Maybe somewhere in my message handling/ que some handling goes on in the background and that's why I get the error.

 

Update;

I also get the access violation when I try to set the CScene with this code in the beginning of the application, when nothing is done yet with the object.

So strange, since it all works fine with my camera class/ object...

Edited by cozzie

Share this post


Link to post
Share on other sites

Getting there, it's compiling and functionally working.

Unfortunately a ***load of unfreed memory, not sure if 'destructing' is working fine when I 'reinitalize' the object with 'CScene = engine_IO::CScene();

 

When I call the destructor manually, right before 're-initializing' the object, there are no memory leaks at all...

 

When I debug the following happens after the line 'CScene = engine_IO:CScene();

 

- first the constructor is called

- then the destructor is called

 

I would expect (and need smile.png) it the other way around..

 

My suspicion is that the default assignment operator is not working out well, because my CScene class has members that need 'deep copy', pointers/ array of other classes etc. So 2 options (?)

- write a big and clean assignment operator

- go for new/ delete and continue doing fun stuff

Edited by cozzie

Share this post


Link to post
Share on other sites

If you're doing manual resource management, then no, the default assignment operator won't work. Your class is clearly in violation of the Rule of Three, you should read up on it.

Share this post


Link to post
Share on other sites
Thanks, clear.
So because I have a non-standard/ own destructor, I also need a DIY copy constructor and assignment operator. In my destructor I delete dynamic arrays which I create when loading a scene. These pointer arrays are set to NULL in the constructor.

All is, IF i use the assignment (=) and copy operators. Which I'm not sure off, but never the less it's good practice.

A few questions:
- can I prevent that in the assignment operator function of the class, the destructor is called at the end of the function (when I return *this)?
- how would the destructors of the member objects of other objects in the class/objects be called, from within my assignment operator function?
- do I have to 'set' every variable manually in the assignment operator function or can I just call the constructor for the new object? Since I want to be able to initiate the object to it's initial values (as already defined in the constructor)?
Or probably I need two versions, one with no function parameter, assigning everything as it is in the constructor, and one version taking over all members from the object in the function parameter.

Sorry for the maybe stupid questions, like to understand how to do the exercise right and efficient.

Share this post


Link to post
Share on other sites


In my destructor I delete dynamic arrays which I create when loading a scene. These pointer arrays are set to NULL in the constructor.

 

Use std::vector instead of your own dynamic array. If you do, the destructor, copy constructor and assignment operator would automatically do the right thing for you and you don't need to write them.

Share this post


Link to post
Share on other sites
Thanks Alvaro.
I never found any good reasons to use std::vector, up till now that is :)

I'll get into it. Always thought I'd be wasting memory when I "reserve" to much elements versus giving in performance when not reservering and only use "push back". Maybe I'll do a new reserve of the vectors size +10 every time I've pushed back 10 more elements. To prevent unnecessary copying. Although this 'feels' like using the advantages of std::vector only partially.

That's why up till now I used dynamic arrays with the size I need. The advantage of the std::vector though, is that for example when loading my scene from file, I don't have to do it twice, once for just counting (determine size of dynamic arrays) and once for the actual loading.

If I go for this I think I also have to adopt iterators and use the size() function, instead of looping up to for example mNrLighs or mNrMeshes. I'll probably keep them to not depend on only the size() of each vector. To prevent that all my source files throughout my engine are required to use vectors. Unless my const functions returning for example the number of lights, meshes etc, just return the vector.size() value.

That's quite a lot of changes throughout the whole engine, but with the advantage being good practice and creating future flexibility.

Share this post


Link to post
Share on other sites


If I go for this I think I also have to adopt iterators and use the size() function, instead of looping up to for example mNrLighs or mNrMeshes. I'll probably keep them to not depend on only the size() of each vector. To prevent that all my source files throughout my engine are required to use vectors. Unless my const functions returning for example the number of lights, meshes etc, just return the vector.size() value.

You can iterate a vector without iterators, the same as you would an array, since it provides a subscript operator and at() function. The only difference in your code would be switching from a manually controlled size to the vector's .size(). Of course, iterators are nice because should you ever decide to switch from a vector to some other standard container, you don't have to refactor all your code again.

Share this post


Link to post
Share on other sites

Thanks Alvaro.
I never found any good reasons to use std::vector, up till now that is smile.png

I'll get into it. Always thought I'd be wasting memory when I "reserve" to much elements versus giving in performance when not reservering and only use "push back". Maybe I'll do a new reserve of the vectors size +10 every time I've pushed back 10 more elements. To prevent unnecessary copying. Although this 'feels' like using the advantages of std::vector only partially.

That's why up till now I used dynamic arrays with the size I need. The advantage of the std::vector though, is that for example when loading my scene from file, I don't have to do it twice, once for just counting (determine size of dynamic arrays) and once for the actual loading.

 If you already use your own dynamic vectors with the size you need, then why not use that size to reserve space for the vector as well?

 

If I go for this I think I also have to adopt iterators and use the size() function, instead of looping up to for example mNrLighs or mNrMeshes. I'll probably keep them to not depend on only the size() of each vector. To prevent that all my source files throughout my engine are required to use vectors. Unless my const functions returning for example the number of lights, meshes etc, just return the vector.size() value.

That's quite a lot of changes throughout the whole engine, but with the advantage being good practice and creating future flexibility.

It's a bad idea to keep track of the size yourself when the vector is already doing that. It introduces more points of failures when you have to ensure that your code is, in fact, always up to date with the vectors: the vectors do that themselves automatically. Switching from your own dynamic arrays to using standard vectors is not something you should just do halfway.

 

And as Zipster said, you don't have to use iterators to access the vector. You probably already use pointer or array subscript access your own dynamic array, and those methods can still be used with the vector.

Share this post


Link to post
Share on other sites
Thanks.
Regarding the 1st point, I do have the requested sizes now, but they're gathered going through a file once, and then loaded afterwards, reading the file again.
I think with std::vector I should be able to do that at once, with each next object just 'push back' the next one. And then in the end I know the total number.
When I'm thinking about this, this is probably the answer. After loading I can use the size() function of each array in my const functions, returning the number of meshes, lights etc.

The only disadvantage is when I load the file directly and fill the objects in the array, I don't know the size on forehand, meaning I have to do some reserve actions (maybe after each 10 objects or so). Unless the disadvantage of copying the array of objects, when I exceed the size of the std::vector, is not that bad (since it's only during loading time).

A few last questions:
- is there some sort of default size that's reserved when initializing the std::vector as member of my class?
- will reserving 'x' objects of the std::vector's in my constructor kill the possibility to use the default assignment operator, copy operator and destructor?
- what's more preferable when I "fill" the objects in the std::vector

1. Initialize them first (calling the constructor) and during filling them:
MyVector.member = value, just loop through 'i'

2. Create a local scope object, same object as the vector has.
Fill the local object and then push_back(local object)

Option 1 seems mostly logical when I know the requested number of objects, or else I'd be contructing unnecessary objects (wasting memory). And option 2 when I just continue till I'm done, now knowing how manh objects there will be.

Share this post


Link to post
Share on other sites

Thanks.
Regarding the 1st point, I do have the requested sizes now, but they're gathered going through a file once, and then loaded afterwards, reading the file again.
I think with std::vector I should be able to do that at once, with each next object just 'push back' the next one. And then in the end I know the total number.
When I'm thinking about this, this is probably the answer. After loading I can use the size() function of each array in my const functions, returning the number of meshes, lights etc.
 
The only disadvantage is when I load the file directly and fill the objects in the array, I don't know the size on forehand, meaning I have to do some reserve actions (maybe after each 10 objects or so). Unless the disadvantage of copying the array of objects, when I exceed the size of the std::vector, is not that bad (since it's only during loading time).

The question is then: is it more expensive to parse the file once to figure out the number of items, or to relocate the vector a few times?

 

By the way, is there some sort of default size that's reserved when initializing the std::vector as member of my class?

Visual Studio 2012 appears to not pre-reserve anything by default, and is expanding the vector by 50% every time it has to reserve more memory. This exponential growth is very efficient in the long run as opposed to just reserve an additional fixed number of items as you mentioned in an earlier post.

 

Will reserving 'x' objects of the std::vector's in my constructor kill the possibility to use the default assignment operator, copy operator and destructor?

No, the vector won't stop working because you tell it to reserve memory before adding items to it.

Share this post


Link to post
Share on other sites
Thanks. Enough theory and preperation for now, I'll just give it a go (and let you know the results).
Thanks again for the help

Share this post


Link to post
Share on other sites

That wasn't so bad, having done the right preparations smile.png

I've implemented it now for my engine::scene class (IO), all working fine, and no problems anymore with the default assignment/ copy operator. Even following the rule of three, DIY destructor not needed anymore.

 

A few conclusions/ thaughts:

 

- I assume there's nothing to do with a vector in my constructor (not setting to NULL or so)

- vector.reserve function is useless when I simply use vector.resize, because in my/ this case I know the exact number/ size

(if I append an object later on, I'll just use 'push back' and it will reserve space for me anyway)

- no forward declarations possible anymore in my header files of the class, because the vector types are no longer pointers

(more includes needed in header file)

- for loops get less readable when using vector.size, returns a vector<int>::size_type instead of regular int

example: for(vector<int>::size_type mi=0;mi<mMeshInstances.size();++mi)

I might consider to use iterators later on, but for now this works fine (no signed/unsigned warnings)

 

Struggle; when going through my scene file the 1st time and count the needed sizes for lights, meshes, etc etc., I have to save these numbers. So basically I'm using both my member variabel int's to find number of meshes, lights etc.. But afterwards I can use vector.size() to go through all objects. Not sure how I can eliminate having the int's as member vars having the same value as the vector's.size(). Unless I change my allocate memory function with a huge number of input parameters).

 

Any input is appreciated as always.

Edited by cozzie

Share this post


Link to post
Share on other sites

- I assume there's nothing to do with a vector in my constructor (not setting to NULL or so)
- vector.reserve function is useless when I simply use vector.resize, because in my/ this case I know the exact number/ size
(if I append an object later on, I'll just use 'push back' and it will reserve space for me anyway)
- no forward declarations possible anymore in my header files of the class, because the vector types are no longer pointers
(more includes needed in header file)

More or less correct.
 

- for loops get less readable when using vector.size, returns a vector<int>::size_type instead of regular int
example: for(vector<int>::size_type mi=0;mi<mMeshInstances.size();++mi)
I might consider to use iterators later on, but for now this works fine (no signed/unsigned warnings)

Well, see Waterlimon's response any my comment below...

 

Struggle; when going through my scene file the 1st time and count the needed sizes for lights, meshes, etc etc., I have to save these numbers. So basically I'm using both my member variabel int's to find number of meshes, lights etc.. But afterwards I can use vector.size() to go through all objects. Not sure how I can eliminate having the int's as member vars having the same value as the vector's.size(). Unless I change my allocate memory function with a huge number of input parameters).
 
Any input is appreciated as always.

Sounds like those int variables don't have to be members. If you only use them locally to count, then make them local variables.
 

For your last point, you can use the 'auto' keyword for the loop counter type instead of explicitly typing it out.
eg. auto mi=0;....

If you're using the auto keywords, you can use ranged for loops as well.

for(auto &element : mMeshInstance) {
}

Share this post


Link to post
Share on other sites

Thanks, I'll definately go for the 'auto' keyword.

Regarding the counters, I see 2 options when I keep them local scope:

 

1. pass all of them through to as function parameters to my AllocateMemory member function (which does the resizes)

2. do the 'resizes' within the function where I go through the file

(3. create a simple struct to save all counters and pass the struct through to the allocatememory function)

 

I like the idea of having separate functions for separate clear goals, like 'AllocateMemory'.

Not such a biggy, I'll choose one of the options and go on creating nice new stuff

 

Ps; just finished all changes for the d3dscene class also, worked out quite well.

With one minor bump, I had to change a header in the 'vector' (header) file, because aligned input was not alowed in the 'resize' function:

	void resize(size_type _Newsize, const _Ty &_Val)
//	void resize(size_type _Newsize, _Ty _Val)

Edited by cozzie

Share this post


Link to post
Share on other sites

Note that the 'auto' keyword requires a C++ 11 compiler. That's generally not a problem on the PC with a modern compiler, but can be on more obscure platforms, or with older compilers.

 

However, I've not yet come across a platform where std::vector<whatever>::size_type isn't equivalent to size_t, and this suggests that the standard says it will always be that way.

 

 

Changing the <vector> header is a very bad plan. It's a system header shared by all projects, and is not designed to be modified at all. If you modify it your code won't work correctly for anyone else. There's almost certainly a correct way to achieve what you want to do, but I'm not sure I understand the problem you're trying to work around by modifying it.

Edited by Adam_42

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement