Merge lp:~hjd/widelands/optimizations into lp:widelands

[Hans Joachim Desserud]

I ran utils/create_cppcheck_report and saw it complained a bit about "Possible inefficient checking for 'variable' emptiness." in a few places. This identifies a few places which check for variable.size(), where it looks like !variable.empty() would be equivelant. (Or better, as size() in worst case will count all elements to give an accurate number, while empty only checks whether it has at least one item or not, which would mean O(n) vs O(1) ) I'm not sure how often these methods are called though, so I don't know whether this would be a noticable performance gain.

Now, c++ is not the language which I'm most proficient in. So to check whether this seems like a good idea, I have pushed this inital patch which fix one file. If people approve of this intial work, I can submit a larger patch which fix more issues in the future.

Also, I'm a bit confused, though this might be due to how cppcheck works. Because I have fixed the two places it complained about in economy/flag.cc, however, size() is used in a similar way at line 72. I guess this isn't listed since it is only a if check, not a while loop which would run the same comparision multiple times. Still, I wonder if checks like this should be changed as well?

[SirVer (sirver)]

I totally approve! In fact, we have a code-style checker rule for
exactly this, seems like it missed these places though.

[Hans Joachim Desserud (hjd)]

Ok, thanks. :) I've pushed some more changes now. Some comments:

If we have a check as part of the stylechecker for this, then I think someone should check whether that still works and is run.

How can logic/building.cc:686 ever be called when the if has the same check as the while loop above?

The cppcheck report also listed logic/tribe.cc:233, but that section looks a bit special and I'm not sure what's going on there, so I left it alone...

[SirVer (sirver)]

>If we have a check as part of the stylechecker for this, then I think someone should check whether that still works and is run.
I was mistaken here. We have a check which checks for the inverse (!blah.size()) which should then
be replaced through blah.empty().

>How can logic/building.cc:686 ever be called when the if has the same check as the while loop above?
There is a break statement in the while loop which will terminate the
loop. So this is valid.

>The cppcheck report also listed logic/tribe.cc:233, but that section looks a bit special and I'm not sure what's going on there, so I left it alone...
It is save to replace this too:
    if (!column.empty())

but it will not make a notable difference in performance. It will only
shut up cppcheck.

[Hans Joachim Desserud (hjd)]

>There is a break statement in the while loop which will terminate the
loop. So this is valid.
Oh, right. I have to admit I only skimmed this section.

=== modified file 'src/economy/flag.cc'
--- src/economy/flag.cc	2012-03-23 16:08:40 +0000
+++ src/economy/flag.cc	2012-04-07 09:48:20 +0000
@@ -499,7 +499,7 @@
 */
 void Flag::wake_up_capacity_queue(Game & game)
 {
-	while (m_capacity_wait.size()) {
+	while (!m_capacity_wait.empty()) {
 		Worker * const w = m_capacity_wait[0].get(game);
 		m_capacity_wait.erase(m_capacity_wait.begin());
 		if (w and w->wakeup_flag_capacity(game, *this))
@@ -714,7 +714,7 @@
 {
 	//molog("Flag::cleanup\n");
 
-	while (m_flag_jobs.size()) {
+	while (!m_flag_jobs.empty()) {
 		delete m_flag_jobs.begin()->request;
 		m_flag_jobs.erase(m_flag_jobs.begin());
 	}

=== modified file 'src/economy/test/test_routing.cc'
--- src/economy/test/test_routing.cc	2012-03-03 19:23:20 +0000
+++ src/economy/test/test_routing.cc	2012-04-07 09:48:20 +0000
@@ -184,7 +184,7 @@
 		nodes.push_back(d1);
 	}
 	~TestingNode_DefaultNodes_Fixture() {
-		while (nodes.size()) {
+		while (!nodes.empty()) {
 			TestingRoutingNode * n = nodes.back();
 			delete n;
 			nodes.pop_back();
@@ -396,7 +396,7 @@
 		nodes.push_back(d0);
 	}
 	~ComplexRouterFixture() {
-		while (nodes.size()) {
+		while (!nodes.empty()) {
 			RoutingNode * n = nodes.back();
 			delete n;
 			nodes.pop_back();

=== modified file 'src/game_io/game_cmd_queue_data_packet.cc'
--- src/game_io/game_cmd_queue_data_packet.cc	2012-02-15 21:25:34 +0000
+++ src/game_io/game_cmd_queue_data_packet.cc	2012-04-07 09:48:20 +0000
@@ -114,7 +114,7 @@
 		// Make a copy, so we can pop stuff
 		std::priority_queue<Cmd_Queue::cmditem> p = cmdq.m_cmds[time % CMD_QUEUE_BUCKET_SIZE];
 
-		while (p.size()) {
+		while (!p.empty()) {
 			Cmd_Queue::cmditem const & it = p.top();
 			if (it.cmd->duetime() == time) {
 				if (upcast(GameLogicCommand, cmd, it.cmd)) {

=== modified file 'src/graphic/richtext.cc'
--- src/graphic/richtext.cc	2012-02-15 21:25:34 +0000
+++ src/graphic/richtext.cc	2012-04-07 09:48:20 +0000
@@ -139,7 +139,7 @@
  */
 void RichTextImpl::clear()
 {
-	while (elements.size()) {
+	while (!elements.empty()) {
 		delete elements.back();
 		elements.pop_back();
 	}

=== modified file 'src/i18n.cc'
--- src/i18n.cc	2012-02-15 21:25:34 +0000
+++ src/i18n.cc	2012-04-07 09:48:20 +0000
@@ -100,7 +100,7 @@
 
 	//don't try to get the previous TD when the very first one ('widelands')
 	//just got dropped
-	if (textdomains.size()) {
+	if (!textdomains.empty()) {
 		char const * const domain = textdomains.back().first.c_str();
 		char const * const localedir = textdomains.back().second.c_str();
 
@@ -235,7 +235,7 @@
 
 	SETLOCALE(LC_ALL, ""); //  call to libintl
 
-	if (textdomains.size()) {
+	if (!textdomains.empty()) {
 		char const * const domain = textdomains.back().first.c_str();
 		char const * const localedir = textdomains.back().second.c_str();
 		bind_textdomain_codeset (domain, "UTF-8");

=== modified file 'src/logic/bob.cc'
--- src/logic/bob.cc	2012-03-02 13:21:49 +0000
+++ src/logic/bob.cc	2012-04-07 09:48:20 +0000
@@ -160,7 +160,7 @@
  */
 void Bob::cleanup(Editor_Game_Base & egbase)
 {
-	while (m_stack.size()) //  bobs in the editor do not have tasks
+	while (!m_stack.empty()) //  bobs in the editor do not have tasks
 		do_pop_task(ref_cast<Game, Editor_Game_Base>(egbase));
 
 	set_owner(0); // implicitly remove ourselves from owner's map
@@ -394,7 +394,7 @@
  */
 void Bob::reset_tasks(Game & game)
 {
-	while (m_stack.size())
+	while (!m_stack.empty())
 		do_pop_task(game);
 
 	m_signal.clear();
@@ -928,7 +928,7 @@
 	game.map().find_bobs
 		(Area<FCoords>(field, 0), &soldiers, FindBobEnemySoldier(get_owner()));
 
-	if (soldiers.size()) {
+	if (!soldiers.empty()) {
 		container_iterate(std::vector<Bob *>, soldiers, i) {
 			Soldier & soldier = ref_cast<Soldier, Bob>(**i.current);
 			if (soldier.getBattle())

=== modified file 'src/logic/building.cc'
--- src/logic/building.cc	2012-02-20 15:54:26 +0000
+++ src/logic/building.cc	2012-04-07 09:48:20 +0000
@@ -666,7 +666,7 @@
 		bool wakeup = false;
 
 		// Wake up one worker
-		while (m_leave_queue.size()) {
+		while (!m_leave_queue.empty()) {
 			upcast(Worker, worker, m_leave_queue[0].get(game));
 
 			m_leave_queue.erase(m_leave_queue.begin());
@@ -682,7 +682,7 @@
 			}
 		}
 
-		if (m_leave_queue.size())
+		if (!m_leave_queue.empty())
 			schedule_act(game, m_leave_time - time);
 
 		if (!wakeup)

=== modified file 'src/logic/cmd_queue.cc'
--- src/logic/cmd_queue.cc	2012-02-15 21:25:34 +0000
+++ src/logic/cmd_queue.cc	2012-04-07 09:48:20 +0000
@@ -113,7 +113,7 @@
 	while (game_time_var < final) {
 		std::priority_queue<cmditem> & current_cmds = m_cmds[game_time_var % CMD_QUEUE_BUCKET_SIZE];
 
-		while (current_cmds.size()) {
+		while (!current_cmds.empty()) {
 			Command & c = *current_cmds.top().cmd;
 			if (game_time_var < c.duetime())
 				break;

=== modified file 'src/logic/game.cc'
--- src/logic/game.cc	2012-03-07 09:39:47 +0000
+++ src/logic/game.cc	2012-04-07 09:48:20 +0000
@@ -1079,7 +1079,7 @@
 
 	const Player_Number nr_players = map().get_nrplayers();
 	iterate_players_existing_novar(p, nr_players, *this)
-		if (m_general_stats.size()) {
+		if (!m_general_stats.empty()) {
 			entries = m_general_stats[p - 1].land_size.size();
 			break;
 		}

=== modified file 'src/logic/immovable.cc'
--- src/logic/immovable.cc	2012-02-15 21:25:34 +0000
+++ src/logic/immovable.cc	2012-04-07 09:48:20 +0000
@@ -1401,7 +1401,7 @@
 */
 void PlayerImmovable::cleanup(Editor_Game_Base & egbase)
 {
-	while (m_workers.size())
+	while (!m_workers.empty())
 		m_workers[0]->set_location(0);
 
 	if (m_owner)

=== modified file 'src/logic/militarysite.cc'
--- src/logic/militarysite.cc	2012-02-23 16:17:50 +0000
+++ src/logic/militarysite.cc	2012-04-07 09:48:20 +0000
@@ -528,7 +528,7 @@
 	std::vector<Soldier *> present = presentSoldiers();
 	Soldier * defender = 0;
 
-	if (present.size()) {
+	if (!present.empty()) {
 		// Find soldier with greatest hitpoints
 		uint32_t current_max = 0;
 		container_iterate_const(std::vector<Soldier *>, present, i)

=== modified file 'src/logic/player.cc'
--- src/logic/player.cc	2012-02-28 22:10:23 +0000
+++ src/logic/player.cc	2012-04-07 09:48:20 +0000
@@ -495,7 +495,7 @@
 	std::vector<OPtr<PlayerImmovable> > bulldozelist;
 	bulldozelist.push_back(&_imm);
 
-	while (bulldozelist.size()) {
+	while (!bulldozelist.empty()) {
 		PlayerImmovable * imm = bulldozelist.back().get(egbase());
 		bulldozelist.pop_back();
 		if (!imm)

=== modified file 'src/logic/production_program.cc'
--- src/logic/production_program.cc	2012-02-28 16:51:08 +0000
+++ src/logic/production_program.cc	2012-04-07 09:48:20 +0000
@@ -412,7 +412,7 @@
 		m_result == Completed ? _("completed") : _("skipped");
 	statistics_string += ' ';
 	statistics_string += ps.top_state().program->descname();
-	if (m_conditions.size()) {
+	if (!m_conditions.empty()) {
 		std::string result_string = statistics_string;
 		if (m_is_when) { //  "when a and b and ..." (all conditions must be true)
 			char const * const operator_string = _(" and ");

=== modified file 'src/logic/productionsite.cc'
--- src/logic/productionsite.cc	2012-02-15 21:25:34 +0000
+++ src/logic/productionsite.cc	2012-04-07 09:48:20 +0000
@@ -719,7 +719,7 @@
 		return true;
 	}
 
-	if (m_produced_items.size()) {
+	if (!m_produced_items.empty()) {
 		//  There is still a produced item waiting for delivery. Carry it out
 		//  before continuing with the program.
 		std::pair<Ware_Index, uint8_t> & ware_type_with_count =
@@ -742,7 +742,7 @@
 		return true;
 	}
 
-	if (m_recruited_workers.size()) {
+	if (!m_recruited_workers.empty()) {
 		//  There is still a recruited worker waiting to be released. Send it
 		//  out.
 		std::pair<Ware_Index, uint8_t> & worker_type_with_count =
@@ -853,7 +853,7 @@
 	std::string const & program_name = top_state().program->name();
 
 	m_stack.pop_back();
-	if (m_stack.size())
+	if (!m_stack.empty())
 		top_state().phase = result;
 
 	switch (result) {

=== modified file 'src/logic/soldier.cc'
--- src/logic/soldier.cc	2012-03-14 21:05:00 +0000
+++ src/logic/soldier.cc	2012-04-07 09:48:20 +0000
@@ -233,60 +233,60 @@
 	std::string run = animation_name;
 
 	if (strcmp(animation_name, "attack_success_w") == 0) {
-		assert(m_attack_success_w_name.size() > 0);
+		assert(!m_attack_success_w_name.empty());
 		uint32_t i = game.logic_rand() % m_attack_success_w_name.size();
 		run = m_attack_success_w_name[i];
 	}
 
 	if (strcmp(animation_name, "attack_success_e") == 0) {
-		assert(m_attack_success_e_name.size() > 0);
+		assert(!m_attack_success_e_name.empty());
 		uint32_t i = game.logic_rand() % m_attack_success_e_name.size();
 		run = m_attack_success_e_name[i];
 	}
 
 	if (strcmp(animation_name, "attack_failure_w") == 0) {
-		assert(m_attack_failure_w_name.size() > 0);
+		assert(!m_attack_failure_w_name.empty());
 		uint32_t i = game.logic_rand() % m_attack_failure_w_name.size();
 		run = m_attack_failure_w_name[i];
 	}
 
 	if (strcmp(animation_name, "attack_failure_e") == 0) {
-		assert(m_attack_failure_e_name.size() > 0);
+		assert(!m_attack_failure_e_name.empty());
 		uint32_t i = game.logic_rand() % m_attack_failure_e_name.size();
 		run = m_attack_failure_e_name[i];
 	}
 
 	if (strcmp(animation_name, "evade_success_w") == 0) {
-		assert(m_evade_success_w_name.size() > 0);
+		assert(!m_evade_success_w_name.empty());
 		uint32_t i = game.logic_rand() % m_evade_success_w_name.size();
 		run = m_evade_success_w_name[i];
 	}
 
 	if (strcmp(animation_name, "evade_success_e") == 0) {
-		assert(m_evade_success_e_name.size() > 0);
+		assert(!m_evade_success_e_name.empty());
 		uint32_t i = game.logic_rand() % m_evade_success_e_name.size();
 		run = m_evade_success_e_name[i];
 	}
 
 	if (strcmp(animation_name, "evade_failure_w") == 0) {
-		assert(m_evade_failure_w_name.size() > 0);
+		assert(!m_evade_failure_w_name.empty());
 		uint32_t i = game.logic_rand() % m_evade_failure_w_name.size();
 		run = m_evade_failure_w_name[i];
 	}
 
 	if (strcmp(animation_name, "evade_failure_e") == 0) {
-		assert(m_evade_failure_e_name.size() > 0);
+		assert(!m_evade_failure_e_name.empty());
 		uint32_t i = game.logic_rand() % m_evade_failure_e_name.size();
 		run = m_evade_failure_e_name[i];
 	}
 	if (strcmp(animation_name, "die_w") == 0) {
-		assert(m_die_w_name.size() > 0);
+		assert(!m_die_w_name.empty());
 		uint32_t i = game.logic_rand() % m_die_w_name.size();
 		run = m_die_w_name[i];
 	}
 
 	if (strcmp(animation_name, "die_e") == 0) {
-		assert(m_die_e_name.size() > 0);
+		assert(!m_die_e_name.empty());
 		uint32_t i = game.logic_rand() % m_die_e_name.size();
 		run = m_die_e_name[i];
 	}
@@ -1185,7 +1185,7 @@
 
 	std::stable_sort(targets.begin(), targets.end(), SoldierDistance::Greater());
 
-	while (targets.size() > 0) {
+	while (!targets.empty()) {
 		const SoldierDistance & target = targets.back();
 
 		if (position == location) {

=== modified file 'src/logic/tribe.cc'
--- src/logic/tribe.cc	2012-03-10 16:58:50 +0000
+++ src/logic/tribe.cc	2012-04-07 09:48:20 +0000
@@ -230,7 +230,7 @@
 				m_ ## w ## s_order_coords[id] = std::pair<uint32_t, uint32_t> \
 					(m_ ## w ## s_order.size(), column.size() - 1); \
 			} \
-			if (column.size()) m_##w##s_order.push_back(column); \
+			if (!column.empty()) m_##w##s_order.push_back(column); \
 		} \
 \
 		/* Check that every ##w## has been added */ \

=== modified file 'src/logic/warehouse.cc'
--- src/logic/warehouse.cc	2012-02-15 21:25:34 +0000
+++ src/logic/warehouse.cc	2012-04-07 09:48:20 +0000
@@ -581,7 +581,7 @@
 		m_portdock = 0;
 	}
 
-	while (m_planned_workers.size()) {
+	while (!m_planned_workers.empty()) {
 		m_planned_workers.back().cleanup();
 		m_planned_workers.pop_back();
 	}
@@ -589,7 +589,7 @@
 	//  all cached workers are unbound and freed
 	container_iterate(IncorporatedWorkers, m_incorporated_workers, cpair) {
 		WorkerList & clist = cpair->second;
-		while (clist.size()) {
+		while (!clist.empty()) {
 			Worker * w = clist.back();
 			clist.pop_back();
 
@@ -1298,7 +1298,7 @@
 
 void Warehouse::PlannedWorkers::cleanup()
 {
-	while (requests.size()) {
+	while (!requests.empty()) {
 		delete requests.back();
 		requests.pop_back();
 	}

=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc	2012-03-06 18:14:55 +0000
+++ src/logic/worker.cc	2012-04-07 09:48:20 +0000
@@ -418,7 +418,7 @@
 				}
 			}
 
-			if (list.size()) {
+			if (!list.empty()) {
 				set_program_objvar
 					(game, state, list[game.logic_rand() % list.size()].object);
 				break;
@@ -443,7 +443,7 @@
 				map.find_reachable_bobs
 					(area, &list, cstep, FindBobAttribute(action.iparam2));
 
-			if (list.size()) {
+			if (!list.empty()) {
 				set_program_objvar
 					(game, state, list[game.logic_rand() % list.size()]);
 				break;

=== modified file 'src/network/nethost.cc'
--- src/network/nethost.cc	2012-03-06 17:15:21 +0000
+++ src/network/nethost.cc	2012-04-07 09:48:20 +0000
@@ -885,7 +885,7 @@
 		clearComputerPlayers();
 		d->game = 0;
 
-		while (d->clients.size() > 0) {
+		while (!d->clients.empty()) {
 			disconnectClient(0, "SERVER_CRASHED");
 			reaper();
 		}
@@ -1252,7 +1252,7 @@
 		// Read in maps
 		std::vector<std::string> directories;
 		directories.push_back("maps");
-		while (directories.size()) {
+		while (!directories.empty()) {
 			filenameset_t files;
 			g_fs->FindFiles(directories.at(directories.size() - 1).c_str(), "*", &files, 0);
 			directories.resize(directories.size() - 1);

=== modified file 'src/ui_basic/box.cc'
--- src/ui_basic/box.cc	2012-02-15 21:25:34 +0000
+++ src/ui_basic/box.cc	2012-04-07 09:48:20 +0000
@@ -100,7 +100,7 @@
 			maxbreadth = breadth;
 	}
 
-	if (m_items.size())
+	if (!m_items.empty())
 		totaldepth += (m_items.size() - 1) * m_inner_spacing;
 
 	if (m_orientation == Horizontal) {
@@ -142,7 +142,7 @@
 		totaldepth += depth;
 	}
 
-	if (m_items.size())
+	if (!m_items.empty())
 		totaldepth += (m_items.size() - 1) * m_inner_spacing;
 
 	bool needscrollbar = false;

=== modified file 'src/writeHTML.cc'
--- src/writeHTML.cc	2012-03-08 22:18:17 +0000
+++ src/writeHTML.cc	2012-04-07 09:48:20 +0000
@@ -902,7 +902,7 @@
 		(m_result == Failed    ? "failed"    :
 		 m_result == Completed ? "completed" :
 		 "skipped");
-	if (m_conditions.size()) {
+	if (!m_conditions.empty()) {
 		std::string op;
 		fw.Text(" <span class=\"keyword\">");
 		if (m_is_when) {

=== modified file 'src/wui/soldierlist.cc'
--- src/wui/soldierlist.cc	2012-02-15 21:25:34 +0000
+++ src/wui/soldierlist.cc	2012-04-07 09:48:20 +0000
@@ -203,7 +203,7 @@
 	}
 
 	// Second pass: add new soldiers
-	while (soldierlist.size()) {
+	while (!soldierlist.empty()) {
 		Icon icon;
 		icon.soldier = soldierlist.back();
 		soldierlist.pop_back();