How dangerous is returning a initialized pointer in C++?

Started by
21 comments, last by Paul C Skertich 9 years, 9 months ago

You wouldn't need to call delete if you would do:


auto engine = std::make_unique<APIEngine>();

I haven't got to smart pointers yet but still I would need the engine pointer for later referencing until the program closes.

That's what std::unique_ptr::get() is for: to get the pointer to the object managed by the unique_ptr without releasing ownership of the pointer.

This tutorial about C++11 smart pointers is worth a read.

Advertisement

What are you trying to accomplish with this design? You could just as easily stack allocate the EngineAPI object in your main function, and then pass around a pointer to that object. This would control the timing of the object creation, it would trivially ensure that the object is destroyed when it goes out of scope, and there is no question of ownership or shared ownership. I usually default to the simplest solution with the fewest strings attached to it, and unless you have a good reason to hide the object creation behind a factory/singleton hybrid method, I think you are unnecessarily complicating this!

You wouldn't need to call delete if you would do:


auto engine = std::make_unique<APIEngine>();

I haven't got to smart pointers yet but still I would need the engine pointer for later referencing until the program closes.

That's what std::unique_ptr::get() is for: to get the pointer to the object managed by the unique_ptr without releasing ownership of the pointer.

This tutorial about C++11 smart pointers is worth a read.

Actually, I think I'm starting to fall in love with smart pointers! I always loved the smart ones. At first it was confusing because I wanted to get the functions I made within that class or struct - you know? I never knew how until I read the tutorial about C++ 11 smart tutorials. I was a bit naive about the std::shared_ptr. So, I tried it on the graphics device struct and there was no worries about ever freeing up the initialized class - just had to dispose the COM objects created by DIrectX. Pretty sweet!

Game Engine's WIP Videos - http://www.youtube.com/sicgames88
SIC Games @ GitHub - https://github.com/SICGames?tab=repositories
Simple D2D1 Font Wrapper for D3D11 - https://github.com/SICGames/D2DFontX

What are you trying to accomplish with this design? You could just as easily stack allocate the EngineAPI object in your main function, and then pass around a pointer to that object. This would control the timing of the object creation, it would trivially ensure that the object is destroyed when it goes out of scope, and there is no question of ownership or shared ownership. I usually default to the simplest solution with the fewest strings attached to it, and unless you have a good reason to hide the object creation behind a factory/singleton hybrid method, I think you are unnecessarily complicating this!

ALl I'm trying to do is making the framework a bit more tidy then I think it is. In a framework shouldn't everything be interchangablly connected as in a car engine or even a tree? I'm probably like you said am putting too much thought in this framework design. I don't think the main ENgineAPI will be a singleton class. The way I see it as of right now - everything's fine I just have a habit of reworking on or redesigning on idea I have.

Game Engine's WIP Videos - http://www.youtube.com/sicgames88
SIC Games @ GitHub - https://github.com/SICGames?tab=repositories
Simple D2D1 Font Wrapper for D3D11 - https://github.com/SICGames/D2DFontX


Actually, I think I'm starting to fall in love with smart pointers! I always loved the smart ones. At first it was confusing because I wanted to get the functions I made within that class or struct - you know? I never knew how until I read the tutorial about C++ 11 smart tutorials. I was a bit naive about the std::shared_ptr. So, I tried it on the graphics device struct and there was no worries about ever freeing up the initialized class - just had to dispose the COM objects created by DIrectX. Pretty sweet!
Smart pointers are a great tool, but remember what they are implying - they implement ownership semantics for the object they are wrapping. If you have a shared pointer to your engine API object, then you are essentially saying that any object that has a reference to it is also an owner of it. That means that if you have one outstanding reference, the object will not be destroyed.

That can be what you want in many cases, but the usage you point out above is not really one of those (at least in my humble opinion!).

What are you trying to accomplish with this design? You could just as easily stack allocate the EngineAPI object in your main function, and then pass around a pointer to that object. This would control the timing of the object creation, it would trivially ensure that the object is destroyed when it goes out of scope, and there is no question of ownership or shared ownership. I usually default to the simplest solution with the fewest strings attached to it, and unless you have a good reason to hide the object creation behind a factory/singleton hybrid method, I think you are unnecessarily complicating this!

ALl I'm trying to do is making the framework a bit more tidy then I think it is. In a framework shouldn't everything be interchangablly connected as in a car engine or even a tree? I'm probably like you said am putting too much thought in this framework design. I don't think the main ENgineAPI will be a singleton class. The way I see it as of right now - everything's fine I just have a habit of reworking on or redesigning on idea I have.

I'm not saying it is right or wrong - I'm just saying that it would be simpler and more safe if you stack allocate the object instead of the method you showed above. There is no more or less dependencies on the implementation of your object in either method, so like I mentioned above, I would tend toward the simplest alternative.


Actually, I think I'm starting to fall in love with smart pointers! I always loved the smart ones. At first it was confusing because I wanted to get the functions I made within that class or struct - you know? I never knew how until I read the tutorial about C++ 11 smart tutorials. I was a bit naive about the std::shared_ptr. So, I tried it on the graphics device struct and there was no worries about ever freeing up the initialized class - just had to dispose the COM objects created by DIrectX. Pretty sweet!
Smart pointers are a great tool, but remember what they are implying - they implement ownership semantics for the object they are wrapping. If you have a shared pointer to your engine API object, then you are essentially saying that any object that has a reference to it is also an owner of it. That means that if you have one outstanding reference, the object will not be destroyed.

That can be what you want in many cases, but the usage you point out above is not really one of those (at least in my humble opinion!).

I'll have to keep that in mind! I don't want this to drag on further because it wasn't my intention. If I'm going to use a smart pointer it may as well be on something that I want to get rid of when it's out of scope or something that isn't referenced a lot of times?

Game Engine's WIP Videos - http://www.youtube.com/sicgames88
SIC Games @ GitHub - https://github.com/SICGames?tab=repositories
Simple D2D1 Font Wrapper for D3D11 - https://github.com/SICGames/D2DFontX

I know Jason - I'm still trying to figure out what's simplier for me as framework design.

Game Engine's WIP Videos - http://www.youtube.com/sicgames88
SIC Games @ GitHub - https://github.com/SICGames?tab=repositories
Simple D2D1 Font Wrapper for D3D11 - https://github.com/SICGames/D2DFontX

All the discussion about smart pointers is great. However, it is possible to make the original code safe (or safer) without using smart pointers. You need to use a static function for Create, instead of a standard function. A static function is part of the class, but doesn't access any of the class members, and thus is safe to use even if you don't have an instantiated object. So, change:


EngineAPI *Create();

To:


static EngineAPI *Create();

And change:


gEngine = gEngine->Create();

To:


gEngine = EngineAPI::Create();

Again, all the smart pointer and ownership discussion is great, but static Create functions are a pattern you'll often see, and are perfectly valid.

Geoff

To make things even better, you could use Meyers Singleton (you're trying to accomplish a singleton, are you not?). It has the advantage of being thread-safe (in C++11), plus: you don't have to use dynamic memory to allocate it, you don't have to always check if you have already allocated it (gEngine != null) and you don't have to deallocate it. It automatically calls your destructor upon program exit.

static EngineAPI * EngineAPI::Create() {
  static EngineAPI engineAPI;
  return &engineAPI;
} 

This topic is closed to new replies.

Advertisement