Fix for the destructor of SoundBuffer.

[BlueCobold]

A crash appeared when a sound still had been attached at the time of destruction.
This is because the destructor iterates over all still attached sounds and calls resetBuffer for each which in return calls Sound::stop() and SoundBuffer::detach() which then removes the element from the vector. Since the destructor is iterating over this very vector, the iterator becomes invalid and calling "++" on it at the end of the loop, caused a crash.

[binary1248]

For those who are wondering, this is the minimal test case that works after the patch is applied:

#include <SFML/Audio.hpp>

int main()
{
    sf::Sound sound;
    sf::SoundBuffer buffer;
    sound.setBuffer(buffer);
}

And on a related note, since this "algorithm" (including the fix) is already employed in SoundBuffer::update(), is there any reason you didn't go with this patch instead?

diff --git a/src/SFML/Audio/SoundBuffer.cpp b/src/SFML/Audio/SoundBuffer.cpp
index cdd5c53..c164cca 100644
--- a/src/SFML/Audio/SoundBuffer.cpp
+++ b/src/SFML/Audio/SoundBuffer.cpp
@@ -65,8 +65,11 @@ m_sounds  () // don't copy the attached sounds
 ////////////////////////////////////////////////////////////
 SoundBuffer::~SoundBuffer()
 {
-    // First detach the buffer from the sounds that use it (to avoid OpenAL errors)
-    for (SoundList::const_iterator it = m_sounds.begin(); it != m_sounds.end(); ++it)
+    // First make a copy of the list of sounds so our iterator doesn't get invalidated
+    SoundList sounds(m_sounds);
+
+    // Detach the buffer from the sounds that use it (to avoid OpenAL errors)
+    for (SoundList::const_iterator it = sounds.begin(); it != sounds.end(); ++it)
         (*it)->resetBuffer();

     // Destroy the buffer

[BlueCobold]

Hehe, no. I didn't check if there's something like this elsewhere. I just got crashes in my game, fixed it and made a PR. If desired, I could either overtake this list-copy-version or you fix it yourself and I discard this PR and branch entirely.
However, I find copying a vector (heap-allocation + copy) in a destructor (and even in SoundBuffer::update for that matter) not a proper or efficient way to deal with it, because there is a cleaner way - although yes, it takes a few more lines of code.

[binary1248]

Since you seem to like performance as much as I do:

// First swap out the list of sounds so our iterator doesn't get invalidated
SoundList sounds;
sounds.swap(m_sounds);

// Detach the buffer from the sounds that use it (to avoid OpenAL errors)
for (SoundList::const_iterator it = sounds.begin(); it != sounds.end();)
{
    (*it)->resetBuffer();
    it = sounds.erase(it);
}

😉

The same idea can be applied to SoundBuffer::update() with the necessary adjustments as well I guess.

Considering your PR is already here, I can't really be bothered creating another PR for something this trivial (changing a diff). Just adopt whatever code fits best. I'm sure the others might have something to say as well.

[BlueCobold]

Sure, lets hear what they have to say. Your swap-version looks good to me though.

[eXpl0it3r]

@binary1248's code seems a bit cleaner, but personally it doesn't really matter, as long as it fixes that issue.

[eXpl0it3r]

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

[LaurentGomila]

I would prefer one of @binary1248's versions, but that's just my personal taste.

[BlueCobold]

I'll change it to @binary1248's swap-version.

[LaurentGomila]

std::move is C++11, swap is not ;)

[BlueCobold]

I already changed that to swap in the meantime.

A C++11 discussion should be eminent though, it is 2015 after all and there are basic features of C++11 which even the lamest compilers should be capable of ;)

[zsbzsb]

Its already been agreed that SFML will continue to support non C++11 compilers until SFML 3 when the C++11 rewrite will take place.

[eXpl0it3r]

The first comment seems a bit misplaced, mind moving that around or adjusting it?

[BlueCobold]

Hu? It isn't misplaced at all.

[eXpl0it3r]

    // First detach the buffer from the sounds that use it (to avoid OpenAL errors)
    // To prevent the iterator to become invalid, move the entire buffer to another
    // container.
    SoundList sounds;
    sounds.swap(m_sounds);
    for (SoundList::const_iterator it = sounds.begin(); it != sounds.end(); ++it)
        (*it)->resetBuffer();

vs

    // To prevent the iterator to become invalid, move the entire buffer to another container.
    SoundList sounds;
    sounds.swap(m_sounds);

    // Detach the buffer from the sounds that use it (to avoid OpenAL errors)
    for (SoundList::const_iterator it = sounds.begin(); it != sounds.end(); ++it)
        (*it)->resetBuffer();

[mantognini]

The latter one is indeed clearer.

[BlueCobold]

Edit:
Will do.

[Bromeon]

I would write a bit more than just that, from the comment it's absolutely not obvious which iterator is invalidated. You should state that resetBuffer() tries to remove the sound from m_sounds, leading to the problem.

And a minor grammar thing: correct would be "prevent from becoming invalid" rather than "prevent to become invalid" :)

[BlueCobold]

Any further requests?

[eXpl0it3r]

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

[eXpl0it3r]

Merged in e78f1bd