Sign in to follow this  
Vincent_M

Canceling a Constructor [C++]

Recommended Posts

Vincent_M    969
Is it a good practice to delete an object in it's constructor? For example, my TGATextureData class uses a constructor to load it's data. It takes in a targa filename, and if my parser supports the format, then it'll load. Now, if this file isn't valid, isn't actually a targa file, or isn't in the format I support, I make my constructor return early. The problem is, I want my constructor to return my object as NULL.

Here's an example:
[code]
class TGATextureData
{
protected:
bool loaded;

void Initialize() { loaded = false; }

public:
TGATextureData();
TGATextureData(const char *filename);
~TGATextureData();

bool IsLoaded() { return loaded; }
};

...


TGATextureData::TGATextureData(const char *filename)
{
Initialize(); // initialize all members
FILE *fp = fopen(filename, "rb");
if(!fp)
{
printf("TGATextureData::TGATextureData(%s): invalid file\n", filename);
// delete this; // not a good idea...
return; // cancel out early
}

// continue loading the data from the file
loaded = true;
}

...

TGATexture *tgaData = new TGATextureData("texture.tga"); // texture.tga does not exist!
[/code]

The code there returns if the supplied filename cannot be opened, or does not exist. What I'd like to do is have my "new TGATextureData('texture.tga')" call return NULL, and not allocate anything at all if something's not correct, but is that possible?

I have a solution to this, but it's a C-based approach. It involves creating a wrapper function outside of the class which tests if construction failed or not:
[code]
TGATextureData *LoadTGATextureData(const char *filename)
{
TGATextureData *tgaData = new TGATextureData(filename);
if(!tgaData->IsLoaded())
{
delete tgaData;
tgaData = NULL;
}
return tgaData;
}

...

TGATextureData *myTexture = LoadTGATextureData("texture.tga"); // returns NULL because "texture.tga" doesn't exist!
[/code]

What that does is load the data to a temporary pointer in the stack. Once constructed, the "loaded" flag is tested if it's loaded or not. If loaded, the pointer to the allocated data is returned as output, or if it's not properly loaded, the object is released from memory, and a NULL pointer is passed. Does this approach seem like good practice for a C++ programmer, or are there better ways of doing this?

Share this post


Link to post
Share on other sites
BloodWarrior    164
Thats constructors for you.
It allocates the memory and runs whatever code for intiialisation. returning null is impossible.
What you usually do is:

a) use the factory method (you used it on the second part)

or

b) throw an exception during the constructor. Use a three stage build for this: constructor intiialises the entire object but can fail and call terminate and then throw. destructor calls terminate. Terminate is called whenever you want to destroy the object (FROM the destructor! dont make this one public!).
Its very important that you deallocate anythng you might allocate during the constructor before you throw yourself out of the constructor! If you dont you will end up with mem leaks.

Does anyone else have a better process than this? I dont particularly fancy the try catch penalty that this requires but I never saw a better solution (other than factory methods).

Yours Truly
K

Share this post


Link to post
Share on other sites
BloodWarrior    164
Oh... yeah.. forgot c)
c) make your constructors fail safe! IE: pre process any resources required for them before hand and pass them directly to the constructor... that way the constructor never fails and there isnt anything that can fail.
But c) is just moving the internal code of the constructor outside and testing it before doing the creation... not really a solution.

Share this post


Link to post
Share on other sites
smasherprog    568
What you are trying to do with a constructor is against how c++ defines constructors. Your solution is to use the constructor to initialize your data members to some default state, then write a function called, say

bool Create(std::string filename);

And have create return true of false depending on if it succeeded or not.

or, do the work in the constructor, but check the object afterwards to see if it worked correctly.

TGATexture *tgaData = new TGATextureData("texture.tga"); // texture.tga does not exist!
if(!tgaData->succeeded) delete tgaData;// failed free the resources

Share this post


Link to post
Share on other sites
XXChester    1364
I think a better approach would be to throw a custom exception when the file is not valid and than catch that exception on the other side and handle it whatever way you want. That way your object will not be constructed and it is the proper way to handle scenario's like this.

Just my 2 cents, hope it helps.

Share this post


Link to post
Share on other sites
Ftn    462
How about:

[code]
class TGATextureData
{
public:
TGATextureData(const std::vector<char>& data)
: m_data(data)
{
}

private:
std::vector<char> m_data;
}

TGATextureData* TryLoadTGATextureDataFromFile(const char *filename)
{
std::vector<char> data;
// loading the data from the file
if (!data.empty())
return new TGATextureData(data);
else
return 0;
}
[/code]

Now your texture data class is more simpler. It is always in usable state, so you don't have to check IsLoaded() anymore. It is not tied to the source of the resource (the filesystem), and you could even drop the TGA part from it.

PS. Why not use RAII?

Share this post


Link to post
Share on other sites
Adaline    710
Maybe you can put in your constuctor all the code that cannot fails, and in a[i] static bool create().... [/i]function the code that can fail ?

Share this post


Link to post
Share on other sites
SriLumpa    198
I think throwing an exception is the correct way to indicate a failure of construction.
Along with using RAII in order not to leak resources (fp in your case).

Share this post


Link to post
Share on other sites
Adaline    710
[quote name='SriLumpa' timestamp='1314981091' post='4856810']
I think throwing an exception is the correct way to indicate a failure of construction.
Along with using RAII in order not to leak resources (fp in your case).
[/quote]

I think handling exceptions in C++ is more expensive than simply check a boolean value .... (?)

Share this post


Link to post
Share on other sites
SriLumpa    198
[quote name='NicoLaCrevette' timestamp='1314981338' post='4856812']
I think handling exceptions in C++ is more expensive than simply check a boolean value .... (?)
[/quote]

I'm not competent to debate that, but in this case, there is not even the need to debate: the OP didn't say that this code was to be as efficient as possible. The famous "premature optimization etc...".
And I doubt that texture loading code is called often.

Share this post


Link to post
Share on other sites
Adaline    710
Thanks :wink:

I think that loading textures has lots of reasons to fail (file not found, memory problem, etc.. ) That's why I proposed to decompose the 'not failing code ' with the 'potentially failing code', (not only for performance purpose)

Share this post


Link to post
Share on other sites
smasherprog    568
Antheus, you always have great responses. +1

You bring up a good point in that if a texture fails to load, a substitute texture should be used instead. There are many different reasons why a texture load could fail, and each one should be handled differently. I believe it depends on where the texture loading is occurring that will determine how to handle each of these cases. Therefore, it is probably best to have some sort of general function which loads textures, and deals with these potential problems in a central area. So, for example, instead of a call to LoadTGA texture, a function should be created called

MyTexture* LoadTexture(std::string filename);

The function LoadTexture should analyze the filename passed, do a filecheck to see if it exists, then pick which loader to use based on the extension passed. And, if any error occurs, either return a substitute texture, or return NULL.

This will abstract away the nitty gritty, often redundant code of creating textures to a central area, making it easier to deal with any errors down the road.

Share this post


Link to post
Share on other sites
Adaline    710
Seems to me that what you would do in error cases doesn't matter here ? (What the difference in error handling between 'boolean' management and 'exception' management ? You have an error while loading your texture that's it, what you do with it is another problem...)

Share this post


Link to post
Share on other sites
Bregma    9202
You should know and understand what a [url="http://http://en.wikipedia.org/wiki/Class_invariant"]class invariant[/url] is. Why you should have them, what impact they have on your code design.

Having class invariants is one of the fundamental concepts behind object-oriented programming. It's up there with "turn a screw one way it tightens, the other way it loosens" in mechanical engineering. It's just that fundamental.

BTW, you should also read the links at the end of the Wikipedia article.

Share this post


Link to post
Share on other sites
Tachikoma    575
What I do is use a generic Texture class to represent image data, then use helper classes to load them from files. For example:


[code]

class Texture
{
private:

//Your texture data stored in here

public:

//Resets member data
Texture(void);

//Copy constructor to handle deep copy of allocated internal data
Texture(const Texture &Tex);

//Assignment operator to destroy and handle deep copy of allocated internal data
Texture &operator = (const Texture &Tex);

//Destroy allocated data
~Texture(void);

public:

/*
List methods here that:
- Creates texture data according to external parameters (res, pixel format, etc)
- Destroys texture data manually
- Gets parameters, such as attributes and buffer pointers, etc
*/
};


class TGA
{
public:
bool Load(Texture &Tex, const std::string &Path);
bool Save(const Texture &Tex, const std::string &Path);
};

class PNG
{
public:
bool Load(Texture &Tex, const std::string &Path);
bool Save(const Texture &Tex, const std::string &Path);
};

class JPEG
{
public:
bool Load(Texture &Tex, const std::string &Path);
bool Save(const Texture &Tex, const std::string &Path);
};


//Somewhere else in code...
Texture Tex[3];

TGA Tga;
if (!Tga.Load(Tex[0], "foo.tga")) {/* error */}

PNG Png;
if (!Png.Load(Tex[1], "foo.png")) {/* error */}

JPEG Jpeg;
if (!Jpeg.Load(Tex[3], "foo.jpeg")) {/* error */}

[/code]

...something like that.

Share this post


Link to post
Share on other sites
vreality    436
Vincent,

Allocating memory from the heap is a messy expensive process. C++ responds to this by never using the heap unless you explicitly tell it to (by calling "new").

The vast majority of objects exist on the stack. That is, when you call a function, the stack pointer jumps by enough to allow space for every object local to that function. That way it doesn't need to muck around with allocating memory for them, and when the function returns, the pointer jumps back to where it was before the function call and the program "forgets" about the objects that were in that space (which then gets reused by the next function call). Efficient, simple, and no heap allocations to clean up.

The implication though, is that the constructor actually has nothing to do with the allocation of memory for holding the object itself. The actual memory that holds the object [i][b]might [/b][/i]have been allocated by "new", or might be on the stack. [i][b]The constructor doesn't know[/b][/i]. By the time the constructor is run, the object (though yet to be initialized) already exists, so the constructor can't decide not to make one. That's why it doesn't return a pointer ("new" does), and can't return "null".

The basic rule of dynamic memory usage is "if you create it, you're responsible for destroying it", which leads to the generally true, "If you didn't create it, you're not allowed to destroy it. Hell you don't even know if it can be destroyed." Which leads us to, "[i][b]Since the constructor didn't allocate the memory[/b][/i] for the object, the constructor can't deallocate it".

Now programs do routinely pass responsibility for dynamically allocated objects on to others. But in order to allow the object's constructor to assume responsibility for the object's own heap storage, it must only ever be called for a dynamically allocated object. In other words you must somehow never allow one to exist on the stack. And the constructor must then somehow pass the responsibility on to something else if it decided [i][b]not [/b][/i]to destroy itself.

Share this post


Link to post
Share on other sites
iMalc    2466
Nitpick:[quote name='Tachikoma' timestamp='1314985657' post='4856852']
[code] //Resets member data
Texture(void);

//Destroy allocated data
~Texture(void);[/code][/quote]

(void) is an old C throwback. In C++ we just use empty parenthesis. Clearly constructors and destructors don't exist in C, so it's not like this code can be compiled by a C compiler anyway.


My 2c:
The case of a texture not being found is most likely an exceptional circumstance. As such, exceptions are suitable. I would free yourself from the burden of having to check the result of a returned boolean in several places (yes "expensive" can apply to coding time as well as performance) and just put a single try catch block in one place to catch all such exceptions from missing textures.

Share this post


Link to post
Share on other sites
vreality    436
Anyway, +1 to the "Factory". One way to structure it to get the behavior you seem to be after might be something like:

[code]
struct TGATextureData;

class TGATexture
{
private: // Objects of this class can *only* be created by members of this class.
TGATexture(); // Unimplemented.
TGATexture(TGATextureData *); // The factory uses this one.

public: // This factory is the only way to get a TGATexture object.
static TGATexture * GetTexture(char const * filename);
...
};
...

TGATexture * TGATexture::GetTexture(char const * filename)
{
TGATextureData * pTextureData = LoadTGATextureData(filename);
if(pTextureData == nullptr)
return nullptr;

return new TGATexture(pTextureData);
}
...

TGATexture * pTexture = TGATexture::GetTexture(filename);
if(pTexture != nullptr)
{ ... }
[/code]

Where LoadTGATextureData() is a static helper function which can try and fail to load the given file.

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