Original post by sandorlev
i've just commited. there are about 10 lines to delete/fix, maybe some not even commented, so i'm not sure it can be compiled.
Always make sure the repository is in a working state. You'll want to be able to do a checkout at any moment, on any machine, and have it up and running without hassle. Your repository is incomplete (main function? project files/make files?) and I can't find any notion about dependencies. I know you're using SDL, but what version? Ideally, you'll want to add the necessary files to the repository. Otherwise you may find yourself guessing as to what SDL version your 1 or 2 years old project used - because it doesn't work with the latest version.
I also noticed you're stuffing multiple classes into a single file. That's confusing: I wouldn't expect to find the Texture and TextureLoader classes in Screen.h/cpp. Just place them in separate files (and name them according to the class - this makes searching through a project much more predictable and easy). If you end up with lots of files (10 isn't much ;)), organize them in folders - and that would perhaps be an appropriate time to use namespaces: each folder (module) it's own namespace.
In most cases, passing std::strings by const reference is better. It prevents the string from being copied (because you're passing a reference) and the const disallows the function to alter the given string (only const member functions can be called on it - which in turn promise not to alter the string).
Making Texture's destructor virtual is only necessary if you intend to give Texture polymorphic behavior. That is, you're planning to derive classes from Texture, each with it's own specific behavior, but you want to treat them all as Textures. That means, you want some virtual member functions, so the correct version is called depending on the actual type of the texture. In such cases, you need a virtual destructor.
Your Texture class is hiding some useful information, like width, height and bpp. Provide some member functions for those. This also means you can throw away that ScreenData struct - it's only duplicating existing data.
I noticed Screen inherits from Texture. I understand that Screen is a surface that can be blitted to, but you're mixing it with Window functionality: a screen doesn't have a caption or a notion of what 'windowed' is - it's just a surface. A Window class that contains a Texture instance (named 'screen') would fit better here - no inheritance from Texture required.
Perhaps the most glaring problem is your memory management. There are various functions that return pointers to new'ed Texture instances, but I couldn't find a single 'delete' call. You'll want to be very strict with who owns what - generally speaking, the code that allocated resources should also clean them up.
I would also look carefully into your Texture class'es assignment operator (hint: you're missing the copy constructor). What happens if you've got two Texture instances (a and b) and you're assigning b to a, and you're then destroying b? A freed it's own surface and then set it's surface pointer to point to b's surface, but when b is destroyed, it frees it's surface. A still points to that surface. Auch. You performed a shallow copy while you needed to perform a deep copy.
But think about it: does it make sense for textures to be copied and assigned? Probably not, unless you want to copy a texture in order to modify a copy at run-time. If you don't need that, then just declare the assignment operator and copy constructor private. That essentially says: instances of this class can't be copied.
As for your problem, Texture's destructor not being called, it's hard to tell without having a working repository, but I guess it's because you're never cleaning up your textures. Which means you're leaking memory. Remember: anything you 'new' must be 'delete'd.
EDIT: If you can, try to get a hold of a copy of 'Effective C++' by Scott Meyers. I'd say it's a must for anyone who's working with C++.