Virtual Destructors?

[TheComet]

Hey,

I'm very new to SFML, so I don't know the guts of it.

However, I couldn't help but notice that no destructors in any of the classes are declared virtual, which could cause memory leaks when trying to inherit them, and using a base pointer to address the object.

This is an example of what I mean:

class cDerivedTexture : public sf::Texture
{
public:
   cDerivedTexture( void );
   ~cDerivedTexture( void );
private:
   // stuff, not really important
};


// Then, later on...


// make a new texture
sf::Texture* myTexture = new cDerivedTexture();

/* --- SNIP --- */

// now delete the texture
delete myTexture; // Oh oh, destructor of sf::Texture isn't called!

I'm going to assume the destructor sf::Texture::~Texture() plays an important role in freeing up memory of loaded images, but in the above example, it isn't called when deleting the derived texture class.

The fix would be to make it virtual:

// The SFML texture class
class Texture
{
   // destructor of sf::Texture
   virtual ~Texture();

   // other stuff
}

Any thoughts or ideas?

TheComet

[Laurent]

And don't you think that it is a good indication that these classes are not meant to be inherited, and, above all, not used polymorphically?

You can't prepare all your classes for all potential usage. Well, you can, but it's a crazy strategy. You have to define only what is relevant, and what is needed for the intented usage. That's what I do in SFML.

Specifically, sf::Texture should never be derived publicly and used polymorphically. It has no virtual function, so there's no point doing that.

[TheComet]

I wanted to write a texture manager class which would keep track of all loaded textures. There are numerous cases where the same texture is used multiple times, and I wanted to write a class which would keep track of all of the loaded textures and prevent duplicates in memory. For this to happen, each texture needs to store some additional data about it (perhaps the path it was loaded from).

My thoughts were to write another class inheriting the sf::Texture class in order to add more methods and attributes to it, which would look somewhat like this:

class cTexture : public sf::Texture
{
   // inherits everything from sf::Texture

private:
   
   // stores the file path the texture was loaded from so it can be compared with other textures in the future
   std::string filePath;
};

I'm unsure if this is the correct way to approach this, any feedback is appreciated.

[Nexus]

For this to happen, each texture needs to store some additional data about it (perhaps the path it was loaded from).

This does not imply inheritance. You can create a class that contains a sf::Texture member and other data (composition).

You could also have a look at thor::ResourceCache (API doc, tutorial) which provide what you need -- namely a way to avoid duplicate loading. However, I have meanwhile made the experience that this is often overkill, a simple container like a map is already enough for cases where you have clearly-defined ownership semantics.

[TheComet]

Thanks for the link, thor is going to help a great deal with my project.

[kralo9]

Why don't try something like

class Texture{
public:
//some accessors etc
private:
sf::Texture _texture;
std::string _path;
};
 

Store this in a std::vector and you can easily load textures and check if a path is alteady loaded

[JayArby]

It makes a lot more sense to inherit from a Texture than to "contain" it and then duplicate its entire API. Seriously, the destructors should just be virtual. How much of a performance hit could it possibly be? And this is not trying to anticipate every possible situation--it's trying to anticipate the most basic, common of requirements.

[FRex]

You don't have to duplicate API and deleting derived sf::Texture through a base pointer is artificial problem, there is literally no reason to do it, all other classes that have any virtual methods have virtual destructors.

[Laurent]

It makes a lot more sense to inherit from a Texture than to "contain" it and then duplicate its entire API. Seriously, the destructors should just be virtual. How much of a performance hit could it possibly be? And this is not trying to anticipate every possible situation--it's trying to anticipate the most basic, common of requirements.

If you inherit from sf::Texture, then you have to keep the derived type in order to use the extra functions, which means that there's no polymorphism and no issue with the destructor.

But anyway, this is really not the right design. Look at some other popular C++ libraries, can you find one where all the destructors are virtual?

[JayArby]

You're right, there's really no polymorphism, but that isn't the point. It's really just a way to add features to the type. In a language like ruby, you could do this without inheritance. In C++, that isn't possible without wrapping the class. If you are a purist about it, this a "wrong" usage of inheritance. But in practical terms, it works and it makes sense and I don't see any disadvantage in it.

[Laurent]

What's the point of a virtual destructor if there's no polymorphism? And what's the point of polymorphism if there's no virtual function? There's no point.

You write stuff when it makes sense from a design point of view. Not just because "there's no disadvantage to it"

[JayArby]

Well I thought I had explained the point of it: it's essentially encapsulation. The virtual destructor is to avoid memory leaks, as pointed out.

[Laurent]

But why would there be a memory leak if you don't delete the instance polymorphically? Since there's no point doing that, there's no problem.

I don't want to write code that would make people's crazy code work fine. I'd rather suggest them how to write better code.

And public inheritance is the worst possible level of encapsulation. In fact you don't encapsulate anything when you inherit publicly from a class. When you wrap it into a container class, however, you do encapsulate it.

[MorleyDev]

This Is-A/Has-A model is a broken way to look at inheritance. It results in overcomplicated designs. A good way to view it is when you inherit you duplicate the responsibilities. Inheritance is a means through which one class can duplicate and extend the responsibilities of another class.

Sometimes this is good, sometimes you want to duplicate and extend responsibilities. But most of the time you do not want to do this. Code is always at it's simplest and cleanest when it has the fewest responsibilities, so inheritance will almost always make your design more complicated.

This is a situation where the code is made so much simpler by encapsulation, by keeping the myriad of responsibilities of the sf::Texture from polluting your class.

[JayArby]

Sorry, I used the wrong term. I didn't mean encapsulation; I meant aggregation.