1
\$\begingroup\$

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;
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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));
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ It's also possible to overload std::default_delete for SDL_RWops, like in this question. \$\endgroup\$
    – G. Sliepen
    Dec 4, 2020 at 14:52
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Dec 4, 2020 at 21:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.