Jump to content

  • Log In with Google      Sign In   
  • Create Account

Canceling a Constructor [C++]


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
18 replies to this topic

#1 Vincent_M   Members   -  Reputation: 749

Like
0Likes
Like

Posted 02 September 2011 - 06:31 AM

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:
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!

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:
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!

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?

Sponsor:

#2 BloodWarrior   Members   -  Reputation: 164

Like
1Likes
Like

Posted 02 September 2011 - 06:50 AM

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
Pain is Inevitable, Suffering is Optional.Unknown

#3 BloodWarrior   Members   -  Reputation: 164

Like
0Likes
Like

Posted 02 September 2011 - 06:54 AM

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.
Pain is Inevitable, Suffering is Optional.Unknown

#4 smasherprog   Members   -  Reputation: 432

Like
0Likes
Like

Posted 02 September 2011 - 08:47 AM

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
Wisdom is knowing when to shut up, so try it.
--Game Development http://nolimitsdesigns.com: Reliable UDP library, Threading library, Math Library, UI Library. Take a look, its all free.

#5 XXChester   Members   -  Reputation: 933

Like
0Likes
Like

Posted 02 September 2011 - 08:56 AM

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.

Remember to mark someones post as helpful if you found it so.

http://www.BrandonMcCulligh.ca

www.gwnp.ca


#6 Codarki   Members   -  Reputation: 462

Like
2Likes
Like

Posted 02 September 2011 - 09:11 AM

How about:

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;
}

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?

#7 Tournicoti   Prime Members   -  Reputation: 684

Like
0Likes
Like

Posted 02 September 2011 - 10:09 AM

Maybe you can put in your constuctor all the code that cannot fails, and in a static bool create().... function the code that can fail ?

#8 SriLumpa   Members   -  Reputation: 198

Like
0Likes
Like

Posted 02 September 2011 - 10:31 AM

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).

#9 Tournicoti   Prime Members   -  Reputation: 684

Like
0Likes
Like

Posted 02 September 2011 - 10:35 AM

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).


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

#10 SriLumpa   Members   -  Reputation: 198

Like
1Likes
Like

Posted 02 September 2011 - 10:46 AM

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


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.

#11 Tournicoti   Prime Members   -  Reputation: 684

Like
0Likes
Like

Posted 02 September 2011 - 10:51 AM

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)

#12 Antheus   Members   -  Reputation: 2397

Like
6Likes
Like

Posted 02 September 2011 - 10:53 AM

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


Yes, it takes about 4 cycles to push the exception handler on stack.
Then, 20 billion cycles are spent loading the tga file.

Booleans are much faster, they take 2 cycles to check.

Also, since object is allocated on heap, there is a mandatory 200 cycle cache miss penalty between new and constructor call.


Now we have a more important question. If loading fails, what then? Do we exit, in which case performance doesn't matter? Since such application fails fast, the loading phase becomes serial, meaning there is no productive work we can do until everything is loaded so optimizations don't apply.

Or do we continue. If we continue, then checking for error is redundant, whether by exception or flag - we'll continue anyway. If anything, we might want to log the failure to load desired texture. The actual class instance might then use the common practice of defaulting to built-in error texture, such as red square with ERROR printed on it.

I think that loading textures has lots of reason to fail (file not found, memory problem, etc.. )

Run-time part typically has no failure modes. There is exactly nothing end user can do if any of these are a problem.

Either the desired texture is loaded or stub is used. An engine which displays occasional wrong texture is better than one that doesn't run. Whether it's memory, missing file, corrupt data - the spice must flow...

Tools might behave differently, reporting a missing texture, but that would be considered a hard error. Either way, editor must continue not only to be able to open the file, but also preserve missing resources.

#13 smasherprog   Members   -  Reputation: 432

Like
0Likes
Like

Posted 02 September 2011 - 11:09 AM

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.
Wisdom is knowing when to shut up, so try it.
--Game Development http://nolimitsdesigns.com: Reliable UDP library, Threading library, Math Library, UI Library. Take a look, its all free.

#14 Tournicoti   Prime Members   -  Reputation: 684

Like
0Likes
Like

Posted 02 September 2011 - 11:20 AM

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...)

#15 Bregma   Crossbones+   -  Reputation: 5503

Like
2Likes
Like

Posted 02 September 2011 - 11:22 AM

You should know and understand what a class invariant 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.
Stephen M. Webb
Professional Free Software Developer

#16 Tachikoma   Members   -  Reputation: 552

Like
0Likes
Like

Posted 02 September 2011 - 11:47 AM

What I do is use a generic Texture class to represent image data, then use helper classes to load them from files. For example:



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 */}


...something like that.
Latest project: Sideways Racing on the iPad

#17 VReality   Members   -  Reputation: 436

Like
0Likes
Like

Posted 03 September 2011 - 01:39 AM

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 might have been allocated by "new", or might be on the stack. The constructor doesn't know. 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, "Since the constructor didn't allocate the memory 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 not to destroy itself.

#18 iMalc   Crossbones+   -  Reputation: 2324

Like
0Likes
Like

Posted 03 September 2011 - 02:24 PM

Nitpick:

//Resets member data
   Texture(void);
   
   //Destroy allocated data
   ~Texture(void);


(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.
"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

#19 VReality   Members   -  Reputation: 436

Like
0Likes
Like

Posted 04 September 2011 - 04:02 AM

Anyway, +1 to the "Factory". One way to structure it to get the behavior you seem to be after might be something like:

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)
{  ...  }

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




Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS