Merge lp:~csirkeee/widelands/bug1150517_3 into lp:widelands

[Kiscsirke]

A fix for bug 1150517 that makes freeing the data conditional on the version of SDL_Mixer.

[SirVer (sirver)]

Btw, is this bug tracked in some sdl bug tracker? I would be interested how this came to be - it seems quite a harsh change in the interface to suddenly change if the caller needs to free his data or not. Especially for a minor version bump.

#include <utility>: please put non-widelands includes first.

otherwise LGTM.

review: Approve

[Kiscsirke (csirkeee)]

Okay, merged.

Well, it seems like they were inconsistent, and were freeing it for some kinds of music, and not for others, so they just fixed that inconsistency: http://bugzilla.libsdl.org/show_bug.cgi?id=1021 and http://hg.libsdl.org/SDL_mixer/rev/565549e046b0 . SDL_mixer seems to be a pretty small/disorganised project :) They still could've warned people better I guess.

[Nicolai Hähnle (nha)]

> #include <utility>: please put non-widelands includes first.

Just for the record, I disagree with this at least in one case. For the common pattern where you have a pair of .h and .cc files that are matched to each other (e.g. because they implement some class), the .cc should have the corresponding .h file as the _first_ include.

This simple trick ensures that you can always include the .h file from any other source file without running into any trouble caused by undefined symbols.

For everything else, I think header files should be written in such a way that the order does not matter.

[SirVer (sirver)]

Well, there are two takes on it: the one we are currently using (setting the corresponding header last) means that you (likely) need to include less headers in the .h which means less stat()ing and less read()ing of headers which are then discluded by include guards. We choose this for Widelands because it actually mattered back then - remember? (I am not sure if this was a concise decision or if we just gravitated towards this automatically).

The other approach that you mention is more modern and more common these days - it has the advantage to have 'less surprises' when you forget to include a header somewhere. I do not really care about the convention - if you feel strongly about it, feel free to change it, but then make sure to also add a style checker rule for the new behavior.

=== modified file 'src/sound/songset.cc'
--- src/sound/songset.cc	2013-02-25 10:41:34 +0000
+++ src/sound/songset.cc	2013-03-19 20:50:32 +0000
@@ -25,6 +25,21 @@
 
 #include "log.h"
 
+#include <utility>
+
+namespace {
+	// The behaviour of whether SDL_Mixer frees the RW it uses was changed with SDL_Mixer version 1.2.12, this
+	// check is so that we don't have a memory leak in the new version.
+	// TODO: Once we can demand that everyone use SDL_Mixer version >= 1.2.12, this function should be removed,
+	// and all usages replaced supposing it's true.
+	bool have_to_free_rw() {
+		return
+			std::make_pair
+				(SDL_MIXER_MAJOR_VERSION, std::make_pair(SDL_MIXER_MINOR_VERSION, SDL_MIXER_PATCHLEVEL)) >=
+			std::make_pair(1, std::make_pair(2, 12));
+	}
+}
+
 /// Prepare infrastructure for reading song files from disk
 Songset::Songset() : m_m(0), m_rwops(0) {}
 
@@ -37,7 +52,8 @@
 		Mix_FreeMusic(m_m);
 
 	if (m_rwops) {
-		SDL_FreeRW(m_rwops);
+		if (have_to_free_rw())
+			SDL_FreeRW(m_rwops);
 		m_fr.Close();
 	}
 }
@@ -82,7 +98,8 @@
 	}
 
 	if (m_rwops) {
-		SDL_FreeRW(m_rwops);
+		if (have_to_free_rw())
+			SDL_FreeRW(m_rwops);
 		m_rwops = 0;
 		m_fr.Close();
 	}