widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/network-memory into lp:widelands.

Commit message:
InternetGaming::games() and InternetGaming::clients() now return nullptr instead of a new vector in case of a communication failure.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/network-memory/+merge/286162

This is a fix for a potential memory leak flagged up by Hasi50.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/network-memory into lp:widelands.

=== modified file 'src/network/internet_gaming.cc'
--- src/network/internet_gaming.cc	2016-02-15 12:35:10 +0000
+++ src/network/internet_gaming.cc	2016-02-16 11:50:52 +0000
@@ -721,10 +721,9 @@
 
 
 
-/// \returns the tables in the room, if no error occured
-const std::vector<InternetGame> & InternetGaming::games() {
-	// TODO(Hasi50): in case of error() this is a memory leak? should return some unmodifiable singleton.
-	return error() ? * (new std::vector<InternetGame>()) : gamelist_;
+/// \returns the tables in the room, if no error occured, or nullptr in case of error
+const std::vector<InternetGame>* InternetGaming::games() {
+	return error() ? nullptr : &gamelist_;
 }
 
 
@@ -739,10 +738,9 @@
 
 
 
-/// \returns the players in the room, if no error occured
-const std::vector<InternetClient> & InternetGaming::clients() {
-	// TODO(Hasi50): in case of error() this is a memory leak? should return some unmodifiable singleton.
-	return error() ? * (new std::vector<InternetClient>()) : clientlist_;
+/// \returns the players in the room, if no error occured, or nullptr in case of error
+const std::vector<InternetClient>* InternetGaming::clients() {
+	return error() ? nullptr : &clientlist_;
 }
 
 

=== modified file 'src/network/internet_gaming.h'
--- src/network/internet_gaming.h	2016-02-15 12:35:10 +0000
+++ src/network/internet_gaming.h	2016-02-16 11:50:52 +0000
@@ -86,9 +86,9 @@
 
 	// Informative functions for lobby
 	bool update_for_games();
-	const std::vector<InternetGame>   & games();
+	const std::vector<InternetGame>* games();
 	bool update_for_clients();
-	const std::vector<InternetClient> & clients();
+	const std::vector<InternetClient>* clients();
 
 	/// sets the name of the local server as shown in the games list
 	void set_local_servername(const std::string & name) {gamename_ = name;}

=== modified file 'src/ui_fsmenu/internet_lobby.cc'
--- src/ui_fsmenu/internet_lobby.cc	2016-02-07 16:31:06 +0000
+++ src/ui_fsmenu/internet_lobby.cc	2016-02-16 11:50:52 +0000
@@ -176,11 +176,13 @@
 		InternetGaming::ref().handle_metaserver_communication();
 	}
 
-	if (InternetGaming::ref().update_for_clients())
+	if (InternetGaming::ref().update_for_clients()) {
 		fill_client_list(InternetGaming::ref().clients());
+	}
 
-	if (InternetGaming::ref().update_for_games())
+	if (InternetGaming::ref().update_for_games()) {
 		fill_games_list(InternetGaming::ref().games());
+	}
 }
 
 void FullscreenMenuInternetLobby::clicked_ok()
@@ -206,30 +208,35 @@
 
 
 /// fills the server list
-void FullscreenMenuInternetLobby::fill_games_list(const std::vector<InternetGame> & games)
+void FullscreenMenuInternetLobby::fill_games_list(const std::vector<InternetGame>* games)
 {
-	// List and button cleanup
-	opengames.clear();
-	hostgame.set_enabled(true);
-	joingame.set_enabled(false);
-	std::string localservername = servername.text();
-	for (uint32_t i = 0; i < games.size(); ++i) {
-		const Image* pic;
-		if (games.at(i).connectable) {
-			if (games.at(i).build_id == build_id())
-				pic = g_gr->images().get("images/ui_basic/continue.png");
-			else {
-				pic = g_gr->images().get("images/ui_basic/different.png");
-			}
-		} else {
-			pic = g_gr->images().get("images/ui_basic/stop.png");
+	if (games != nullptr) { // only update if no communication error occurred.
+		// List and button cleanup
+		opengames.clear();
+		hostgame.set_enabled(true);
+		joingame.set_enabled(false);
+		std::string localservername = servername.text();
+
+		for (uint32_t i = 0; i < games->size(); ++i) {
+			const Image* pic;
+			const InternetGame& game = games->at(i);
+			if (game.connectable) {
+				if (game.build_id == build_id())
+					pic = g_gr->images().get("images/ui_basic/continue.png");
+				else {
+					pic = g_gr->images().get("images/ui_basic/different.png");
+				}
+			} else {
+				pic = g_gr->images().get("images/ui_basic/stop.png");
+			}
+			// If one of the servers has the same name as the local name of the
+			// clients server, we disable the 'hostgame' button to avoid having more
+			// than one server with the same name.
+			if (game.name == localservername) {
+				hostgame.set_enabled(false);
+			}
+			opengames.add(game.name, game, pic, false, game.build_id);
 		}
-		// If one of the servers has the same name as the local name of the
-		// clients server, we disable the 'hostgame' button to avoid having more
-		// than one server with the same name.
-		if (games.at(i).name == localservername)
-			hostgame.set_enabled(false);
-		opengames.add(games.at(i).name, games.at(i), pic, false, games.at(i).build_id);
 	}
 }
 
@@ -259,43 +266,43 @@
 }
 
 /// fills the client list
-void FullscreenMenuInternetLobby::fill_client_list(const std::vector<InternetClient> & clients)
+void FullscreenMenuInternetLobby::fill_client_list(const std::vector<InternetClient>* clients)
 {
-	clientsonline.clear();
-	for (uint32_t i = 0; i < clients.size(); ++i) {
-		const InternetClient & client(clients[i]);
-		UI::Table<const InternetClient * const>::EntryRecord & er = clientsonline.add(&client);
-		er.set_string(1, client.name);
-		er.set_string(2, client.points);
-		er.set_string(3, client.game);
+	if (clients != nullptr) { // only update if no communication error occurred.
+		clientsonline.clear();
+		for (uint32_t i = 0; i < clients->size(); ++i) {
+			const InternetClient& client(clients->at(i));
+			UI::Table<const InternetClient * const>::EntryRecord & er = clientsonline.add(&client);
+			er.set_string(1, client.name);
+			er.set_string(2, client.points);
+			er.set_string(3, client.game);
 
-		const Image* pic;
-		switch (convert_clienttype(client.type)) {
-			case 0: // UNREGISTERED
-				pic = g_gr->images().get("images/wui/overlays/roadb_red.png");
-				er.set_picture(0, pic);
-				break;
-			case 1: // REGISTERED
-				pic = g_gr->images().get("images/wui/overlays/roadb_yellow.png");
-				er.set_picture(0, pic);
-				break;
-			case 2: // SUPERUSER
-			case 3: // BOT
-				pic = g_gr->images().get("images/wui/overlays/roadb_green.png");
-				er.set_color(RGBColor(0, 255, 0));
-				er.set_picture(0, pic);
-				break;
-			default:
-				continue;
+			const Image* pic;
+			switch (convert_clienttype(client.type)) {
+				case 0: // UNREGISTERED
+					pic = g_gr->images().get("images/wui/overlays/roadb_red.png");
+					er.set_picture(0, pic);
+					break;
+				case 1: // REGISTERED
+					pic = g_gr->images().get("images/wui/overlays/roadb_yellow.png");
+					er.set_picture(0, pic);
+					break;
+				case 2: // SUPERUSER
+				case 3: // BOT
+					pic = g_gr->images().get("images/wui/overlays/roadb_green.png");
+					er.set_color(RGBColor(0, 255, 0));
+					er.set_picture(0, pic);
+					break;
+				default:
+					continue;
+			}
 		}
-	}
 
-	// If a new player joins the lobby, play a sound.
-	if (clients.size() != prev_clientlist_len_)
-	{
-		if (clients.size() > prev_clientlist_len_ && !InternetGaming::ref().sound_off())
+		// If a new player joins the lobby, play a sound.
+		if (clients->size() > prev_clientlist_len_ && !InternetGaming::ref().sound_off()) {
 			play_new_chat_member();
-		prev_clientlist_len_ = clients.size();
+		}
+		prev_clientlist_len_ = clients->size();
 	}
 }
 
@@ -359,10 +366,13 @@
 
 	// Check whether a server of that name is already open.
 	// And disable 'hostgame' button if yes.
-	const std::vector<InternetGame> & games = InternetGaming::ref().games();
-	for (uint32_t i = 0; i < games.size(); ++i) {
-		if (games.at(i).name == servername.text())
-			hostgame.set_enabled(false);
+	const std::vector<InternetGame>* games = InternetGaming::ref().games();
+	if (games != nullptr) {
+		for (uint32_t i = 0; i < games->size(); ++i) {
+			if (games->at(i).name == servername.text()) {
+				hostgame.set_enabled(false);
+			}
+		}
 	}
 }
 

=== modified file 'src/ui_fsmenu/internet_lobby.h'
--- src/ui_fsmenu/internet_lobby.h	2016-02-07 16:31:06 +0000
+++ src/ui_fsmenu/internet_lobby.h	2016-02-16 11:50:52 +0000
@@ -63,8 +63,8 @@
 	const char * password;
 	bool         reg;
 
-	void fill_games_list (const std::vector<InternetGame> &);
-	void fill_client_list(const std::vector<InternetClient> &);
+	void fill_games_list (const std::vector<InternetGame>*);
+	void fill_client_list(const std::vector<InternetClient>*);
 
 	void connect_to_metaserver();