C++ - Sorting vector of structs with std::sort results in read access violation

Question

I have a problem with the std::sort-method. In the following code I'm using the std::sort-method to sort a vector of structs (= Highscore). However, when I run this line a "read access violation" exception is thrown in the xmemory-file.

Here are the details: Exception thrown: read access violation. _Pnext was 0x217AE3EE9D8. occurred

This is the method where the error occures.

void HighscoreManager::sortAndChangeRanks(bool deleteLast) {
    std::sort(_highscores.begin(), _highscores.end());
    if (deleteLast && _highscores.size() > MaxHighscores) {
        _highscores.pop_back();
    }

    for (int i = 0; i < _highscores.size(); i++) {
        _highscores.at(i).rank = i + 1;
    }
}

_highscores is defined as std::vector<Highscore> _highscores; and is filled with values from a file before the method call. This works just fine. When im debugging right before using the sort-Method, the vector is filled with the right values from the file.

This is the implementation of the Highscore-struct:

struct Highscore {
    int rank;
    std::string name;
    int points;

    Highscore() {}

    Highscore(int r, std::string n, int p) : rank(r), name(std::move(n)), points(p) {}

    bool operator<(const Highscore& h1) const {
        return points < h1.points;
    }
};

Please help me or point me to a direction where the error could lie, I'm out of ideas.

EDIT

Since it was asked in the comments where the vector is used before the call to std::sort, this is the method which is called from the object constructor and the only time the vector is used before the sorting. This way of reading (writing works similarly) from a binary file is based on this.

bool HighscoreManager::loadFromFile() {
    std::ifstream in(FileName, std::ios::in | std::ios::binary);
    if(!in) {
        return false;
    }

    try {
        std::vector<Highscore>::size_type size = 0;
        in.read((char*)&size, sizeof(size));
        _highscores.resize(size);
        in.read((char*)&_highscores[0], _highscores.size() * sizeof(Highscore));        
    } catch(const std::exception& e) {
        std::cout << e.what() << std::endl;
    }

    in.close();
    sortAndChangeRanks(false);
    return in.good();
}

Answer 1

I don’t know what’s “optimized” about your high score storage. It seems like just a waste of effort for nothing. You’re not storing millions of high scores. You could just store them as text. The “optimization” can’t be measured in normal use. And if you think you’re optimizing: show measurements. Otherwise you’re fooling yourself and wasting time.

On top of it, you’ve complicated the code enough to that you ran into a problem that took a long time to debug. That’s a learning experience, but strictly speaking you wasted even more time because of it. Your time costs more than runtime, in most cases.

All you needed was trivial text stream I/O that can be done in two minutes. Messing about with binary storage is not advised if you don’t thoroughly understand what’s going on. As it stands, your code will crash or worse if you try reading the high scores written on a machine with different endianness. And now you got to manage endianness of all the numeric data… good luck.

In any case, it’s actually a pessimization, since you constantly reallocate the temporary string buffer. That buffer is not needed. You should resize the string itself an put the data in it.

std::string name(nLen);
in.read(&name[0], name.size());

Answer 2

Here is the current solution I'm using. This works for me and solves my problem, which is with reading/writing an std::string to a binary file and not with the sorting method (Thanks to the comments on the question!). To fix this problem I used parts of this.

reading from a file:

std::ifstream in(FileName, std::ios::in | std::ios::binary);
    if(!in)    {
        return false;
    }

    try    {
        std::vector<Highscore>::size_type size = 0;
        in.read((char*)&size, sizeof(size));
        for(int i = 0; i < size; i++) {
            int r, p;
            size_t nLen;
            in.read((char*)&r, sizeof(int));
            in.read((char*)&p, sizeof(int));
            in.read((char*)&nLen, sizeof(size_t));

            char* temp = new char[nLen + 1];
            in.read(temp, nLen);
            temp[nLen] = '\0';
            std::string name = temp;
            delete[] temp;

            _highscores.emplace_back(r, name, p);
        }
    } catch(const std::exception& e) {
        std::cout << e.what() << std::endl;
    }

    in.close();
    sortAndChangeRanks(false);
    return in.good();
}

writing to a file:

bool HighscoreManager::saveToFile() {
    std::ofstream out(FileName, std::ios::out | std::ios::binary);
    if(!out) {
        return false;
    }

    std::vector<Highscore>::size_type size = _highscores.size();
    try {
        out.write((char*)&size, sizeof(size));
        for(int i = 0; i < size; i++) {
            out.write((char*)&_highscores.at(i).rank, sizeof(int));
            out.write((char*)&_highscores.at(i).points, sizeof(int));
            size_t nameLen = _highscores.at(i).name.size();
            out.write((char*)&nameLen, sizeof(size_t));
            out.write((char*)_highscores.at(i).name.c_str(), nameLen);
        }
    } catch (const std::exception& e) {
        std::cout << e.what() << std::endl;
    }
    out.close();

    return out.good();
}

Thank you all for your help!