How to use smart pointers with SDL2 (SDL_RWops)?

Question

I want to write read savefile function with smart pointers and SDL2. I have little expirience with smart pointers and just want to ask is my code good, correct and no memory leaks.

this is my code:

std::shared_ptr<SDL_RWops> saveFile;

--

int GameEngine::ReadSave() {
 int Highlevel;
 //Open file for reading in binary
 saveFile = std::shared_ptr<SDL_RWops>(SDL_RWFromFile( "data/save.txt", "r" ), SDL_RWclose);
 //File does not exist
 if( saveFile.get() == NULL ){
 printf( "Warning: Unable to open file! SDL Error: %s\n", SDL_GetError() );
 
 Highlevel = 0;
 //Create file for writing
 saveFile = std::shared_ptr<SDL_RWops>(SDL_RWFromFile( "data/save.txt", "w+" ), SDL_RWclose);
 if( saveFile.get() != NULL ){
 printf( "New save file created!\n" );
 //Initialize data
 SDL_RWwrite( saveFile.get(), &Highlevel, sizeof(int), 1 );
 }
 else {
 printf( "Error: Unable to create savefile! SDL Error: %s\n", SDL_GetError() );
 }
 }
 else{
 //Load data
 printf( "Reading save file...!\n" );
 SDL_RWread( saveFile.get(), &Highlevel, sizeof(int), 1 );
 }
 return Highlevel;
}

Answer 1

Your idea to wrap the result of SDL_RWFromFile in a smart pointer is a good idea. I recommend a few improvements:

---

Factor out the opening-and-wrapping operations into a named function. For example:

auto OpenFile(const char *fname, const char *mode) {
 return std::shared_ptr<SDL_RWops>(SDL_RWFromFile(fname, mode), SDL_RWclose);
}

---

if( saveFile.get() == NULL ){

Your whitespace is all messed up here; and also, you should generally treat smart pointers the same way you'd treat regular pointers. Avoid using .get() and .reset() and so on, unless you have a specific reason you need to emphasize the "object-ness" of the smart pointer. Keep it simple by writing:

saveFile = OpenFile("data/save.txt", "r");
if (saveFile == nullptr) {
 printf("Warning: Unable to open savefile! SDL Error: %s\n", SDL_GetError());
 Highlevel = 0;
 saveFile = OpenFile("data/save.txt", "w+");
 if (saveFile == nullptr) {
 printf("Error: Unable to create savefile! SDL Error: %s\n", SDL_GetError());
 } else {
 printf("New save file created!\n");
 SDL_RWwrite(saveFile.get(), &Highlevel, sizeof(Highlevel), 1);
 }
} else {
 printf("Reading save file...!\n");
 SDL_RWread(saveFile.get(), &Highlevel, sizeof(Highlevel), 1);
}

I notice that you are not checking the return value of SDL_RWread for errors. That's not great.

Have you considered throwing an exception on failure, instead of using printf to report directly to the user? This would certainly streamline your code:

if (auto f = OpenFile("data/save.txt", "r")) {
 printf("Reading save file...\n");
 if (SDL_RWread(f.get(), &Highlevel, sizeof(Highlevel), 1) != 1) {
 throw std::runtime_error(std::string("Save file format error: ") + SDL_GetError());
 }
} else if (auto f = OpenFile("data/save.txt", "w+")) {
 printf("Writing new save file...\n");
 Highlevel = 0;
 SDL_RWwrite(f.get(), &Highlevel, sizeof(Highlevel), 1);
} else {
 throw std::runtime_error(std::string("Unable to create save file: ") + SDL_GetError());
}

---

Finally, shared_ptr is overkill here because you never have more than one "owner" of the open file. You should certainly rewrite OpenFile to return a unique_ptr — a type which you can still implicitly convert to shared_ptr if you do ever need shared ownership of the file. See "OpenSSL client and server from scratch, part 1"; it's basically this:

struct RWCloser { void operator()(SDL_RWops *p) const { SDL_RWclose(p); } };
using UniqueRWops = std::unique_ptr<SDL_RWops, RWCloser>;
UniqueRWops OpenFile(const char *fname, const char *mode) {
 return UniqueRWops(SDL_RWFromFile(fname, mode));
}

• G. Sliepen: It's also possible to overload std::default_delete for SDL_RWops, like in this question.
• Quuxplusone: std::default_delete is not a customization point and should not be customized by the programmer. It's a library facility, like std::less. std::less is the lifting of a<b into a functor; std::default_delete is the lifting of delete p into a functor. It shall not be customized. If you want to specialize unique_ptr<T,D> with a custom D, just do that.