widelands-dev team mailing list archive

[Hans Joachim Desserud <launchpad@xxxxxxxxxxxx>]

Hans Joachim Desserud has proposed merging lp:~hjd/widelands/cppcheck-fixes into lp:widelands.

Requested reviews:
  Widelands Developers (widelands-dev)
Related bugs:
  Bug #986611 in widelands: "Issues reported by cppcheck"
  https://bugs.launchpad.net/widelands/+bug/986611

For more details, see:
https://code.launchpad.net/~hjd/widelands/cppcheck-fixes/+merge/229338

Fixes some cppcheck issues. I have a couple of questions though, see the diff.
(In my lack of creativity, I seem to have reused a branch name. Hopefully this won't cause any issues.)
-- 
https://code.launchpad.net/~hjd/widelands/cppcheck-fixes/+merge/229338
Your team Widelands Developers is requested to review the proposed merge of lp:~hjd/widelands/cppcheck-fixes into lp:widelands.

=== modified file 'src/ai/defaultai.cc'
--- src/ai/defaultai.cc	2014-07-30 17:42:45 +0000
+++ src/ai/defaultai.cc	2014-08-02 18:47:18 +0000
@@ -2650,9 +2650,8 @@
 	const uint16_t attempts = militarysites.size() / 6 + 1;
 	Map& map = game().map();
 
-	uint16_t position = 0;
 	for (uint32_t i = 0; i < attempts && !any_attacked; ++i) {
-		position = (game().get_gametime() + (3 * i)) % militarysites.size();
+		uint16_t position = (game().get_gametime() + (3 * i)) % militarysites.size();
 
 		// picking random military sites
 		// using gametime as a random value, but it is constant so each next is on position +3

=== modified file 'src/logic/production_program.cc'
--- src/logic/production_program.cc	2014-08-01 12:57:17 +0000
+++ src/logic/production_program.cc	2014-08-02 18:47:18 +0000
@@ -1244,7 +1244,6 @@
 	uint32_t totalres    = 0;
 	uint32_t totalchance = 0;
 	uint32_t totalstart  = 0;
-	int32_t pick;
 
 	{
 		MapRegion<Area<FCoords> > mr
@@ -1291,8 +1290,9 @@
 
 		//  second pass through nodes
 		assert(totalchance);
-		pick = game.logic_rand() % totalchance;
+		int32_t pick = game.logic_rand() % totalchance;
 
+		//TODO(code review): why is this section in it's own block?
 		{
 			MapRegion<Area<FCoords> > mr
 				(map,
@@ -1316,8 +1316,9 @@
 			} while (mr.advance(map));
 		}
 
-		if (pick >= 0)
+		if (pick >= 0) {
 			return ps.program_end(game, Failed);
+		}
 
 	} else {
 		//  Inform the player about an empty mine, unless

=== modified file 'src/logic/worker.cc'
--- src/logic/worker.cc	2014-08-01 12:57:17 +0000
+++ src/logic/worker.cc	2014-08-02 18:47:18 +0000
@@ -779,8 +779,6 @@
 			return true;
 		}
 
-	std::vector<bool> is_tribe_specific;
-
 	// Figure the (at most) six best fitting immovables (as judged by terrain
 	// affinity). We will pick one of them at random later. The container is
 	// picked to be a stable sorting one, so that no deyncs happen in
@@ -2829,12 +2827,12 @@
 			(map.get_fcoords(get_position()), state.ivar1);
 		Coords oldest_coords = get_position();
 		Time oldest_time = game.get_gametime();
-		uint8_t oldest_distance = 0;
 
 		// if some fields can be reached
 		if (map.find_reachable_fields(exploring_area, &list, cstep, ffa) > 0) {
 			// Parse randomly the reachable fields, maximum 50 iterations
 			uint8_t iterations = list.size() % 51;
+			uint8_t oldest_distance = 0;
 			for (uint8_t i = 0; i < iterations; ++i) {
 				const std::vector<Coords>::size_type lidx = game.logic_rand() % list.size();
 				Coords const coord = list[lidx];

=== modified file 'src/ui_basic/panel.cc'
--- src/ui_basic/panel.cc	2014-07-26 10:43:23 +0000
+++ src/ui_basic/panel.cc	2014-08-02 18:47:18 +0000
@@ -186,8 +186,6 @@
 	start();
 	g_gr->update_fullscreen();
 
-	uint32_t startTime;
-	uint32_t diffTime;
 	uint32_t minTime;
 	{
 		int32_t maxfps = g_options.pull_section("global").get_int("maxfps", 25);
@@ -197,7 +195,11 @@
 	}
 
 	while (_running) {
-		startTime = SDL_GetTicks();
+		//TODO(code review): By initializing the variables inside the loop, they will
+		//will basically be created, possibly memory allocated and so on over and over again.
+		//Is this a potential concern so that we might want to keep them outside the loop?
+		//It's two ints, so I doubt it will have that much of an impact, but just in case.
+		uint32_t startTime = SDL_GetTicks();
 
 		static InputCallback icb = {
 			Panel::ui_mousepress,
@@ -232,9 +234,10 @@
 			check_child_death();
 
 		//  Wait until 1second/maxfps are over.
-		diffTime = SDL_GetTicks() - startTime;
-		if (diffTime < minTime)
+		uint32_t diffTime = SDL_GetTicks() - startTime;
+		if (diffTime < minTime) {
 			SDL_Delay(minTime - diffTime);
+		}
 	}
 	g_gr->update_fullscreen();
 	end();

=== modified file 'src/wlapplication.cc'
--- src/wlapplication.cc	2014-07-25 22:17:48 +0000
+++ src/wlapplication.cc	2014-08-02 18:47:18 +0000
@@ -1781,7 +1781,12 @@
 	if (path.empty()) {
 #ifdef _WIN32
 		char module_name[MAX_PATH];
-		unsigned int name_length = GetModuleFileName(nullptr, module_name, MAX_PATH);
+		//TODO(code review): the return value of this method was assigned to a variable which was not used.
+		//However, according to the documentation (http://msdn.microsoft.com/en-us/library/windows/desktop/ms683197(v=vs.85).aspx)
+		//it will write the full path into the second parameter which means I shouldn't remove the method call.
+		//Btw, someone should compare this and line 853 which use a different null and MAX_PATH (!?!)
+		//for the first and third parameter. I don't see why they would be different...
+		GetModuleFileName(nullptr, module_name, MAX_PATH);
 		path = module_name;
 		size_t pos = path.find_last_of("/\\");
 		if (pos == std::string::npos) return false;

=== modified file 'src/wui/attack_box.cc'
--- src/wui/attack_box.cc	2014-07-05 14:22:44 +0000
+++ src/wui/attack_box.cc	2014-08-02 18:47:18 +0000
@@ -137,6 +137,8 @@
 }
 
 void AttackBox::init() {
+	//TODO(code review): possible to reduce scope of buf here, though might be easier
+	//to remove the block (and outdent the content of it, of course).
 	char buf[10];
 	assert(m_node);