widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/bug-986611-cppcheck-minimal into lp:widelands.

Commit message:
Various small codestyle fixes flagged up by cppcheck.

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/~widelands-dev/widelands/bug-986611-cppcheck-minimal/+merge/328910

Reviewers should pay attention to the changes in comparisons to make sure that they are what we want logically.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-986611-cppcheck-minimal into lp:widelands.

=== modified file 'src/economy/fleet.cc'
--- src/economy/fleet.cc	2017-07-01 15:36:36 +0000
+++ src/economy/fleet.cc	2017-08-11 11:40:48 +0000
@@ -801,11 +801,10 @@
 	// looking for best scores and sending ships accordingly
 	uint16_t best_ship = 0;
 	uint16_t best_port = 0;
-	uint16_t best_score;
 
 	// after sending a ship we will remove one or more items from scores
 	while (!scores.empty()) {
-		best_score = 0;
+		uint16_t best_score = 0;
 
 		// searching for combination with highest score
 		for (const auto& combination : scores) {

=== modified file 'src/editor/tools/info_tool.cc'
--- src/editor/tools/info_tool.cc	2017-01-25 18:55:59 +0000
+++ src/editor/tools/info_tool.cc	2017-08-11 11:40:48 +0000
@@ -104,8 +104,7 @@
 		buf += std::string("• ") + _("Owned by: —") + "\n";
 	}
 
-	std::string temp = "";
-	temp = f.get_immovable() ? _("Has immovable") : _("No immovable");
+	std::string temp = f.get_immovable() ? _("Has immovable") : _("No immovable");
 	buf += "• " + temp + "\n";
 
 	temp = f.get_first_bob() ? _("Has animals") : _("No animals");

=== modified file 'src/graphic/minimap_renderer.cc'
--- src/graphic/minimap_renderer.cc	2017-05-21 18:15:17 +0000
+++ src/graphic/minimap_renderer.cc	2017-08-11 11:40:48 +0000
@@ -162,7 +162,7 @@
 
 	for (uint32_t y = 0; y < surface_h; ++y) {
 		Widelands::FCoords f(
-		   Widelands::Coords(top_left.x, top_left.y + (layers & MiniMapLayer::Zoom2 ? y / 2 : y)));
+		   Widelands::Coords(top_left.x, top_left.y + ((layers & MiniMapLayer::Zoom2) ? y / 2 : y)));
 		map.normalize_coords(f);
 		f.field = &map[f];
 		Widelands::MapIndex i = Widelands::Map::get_index(f, mapwidth);

=== modified file 'src/graphic/text/bidi.cc'
--- src/graphic/text/bidi.cc	2017-06-19 18:32:06 +0000
+++ src/graphic/text/bidi.cc	2017-08-11 11:40:48 +0000
@@ -618,7 +618,7 @@
 					--i;
 					previous = (i > 0) ? parseme.charAt(i - 1) : not_a_character;
 				}
-			} catch (std::out_of_range e) {
+			} catch (const std::out_of_range& e) {
 				log("Error trying to fetch Arabic diacritic form: %s\n", e.what());
 				NEVER_HERE();
 			}
@@ -644,7 +644,7 @@
 					}
 				}
 				c = find_arabic_letter_form(c, previous, next);
-			} catch (std::out_of_range e) {
+			} catch (const std::out_of_range& e) {
 				log("Error trying to fetch Arabic character form: %s\n", e.what());
 				NEVER_HERE();
 			}

=== modified file 'src/logic/map.cc'
--- src/logic/map.cc	2017-01-30 14:40:12 +0000
+++ src/logic/map.cc	2017-08-11 11:40:48 +0000
@@ -1263,11 +1263,10 @@
 	                                         WALK_SE, WALK_E,  WALK_E,  WALK_E};
 	const FCoords start = br_n(br_n(get_fcoords(c)));
 	const Widelands::PlayerNumber owner = start.field->get_owned_by();
-	bool is_good_water;
 	FCoords f = start;
 	std::vector<Coords> portdock;
 	for (uint32_t i = 0; i < 16; ++i) {
-		is_good_water = (f.field->get_caps() & (MOVECAPS_SWIM | MOVECAPS_WALK)) == MOVECAPS_SWIM;
+		bool is_good_water = (f.field->get_caps() & (MOVECAPS_SWIM | MOVECAPS_WALK)) == MOVECAPS_SWIM;
 
 		// Any immovable here? (especially another portdock)
 		if (is_good_water && f.field->get_immovable()) {
@@ -1654,7 +1653,7 @@
 		// avoid bias by using different orders when pathfinding
 		static const int8_t order1[] = {WALK_NW, WALK_NE, WALK_E, WALK_SE, WALK_SW, WALK_W};
 		static const int8_t order2[] = {WALK_NW, WALK_W, WALK_SW, WALK_SE, WALK_E, WALK_NE};
-		int8_t const* direction = (cur.x + cur.y) & 1 ? order1 : order2;
+		int8_t const* direction = ((cur.x + cur.y) & 1) ? order1 : order2;
 
 		// Check all the 6 neighbours
 		for (uint32_t i = 6; i; i--, direction++) {
@@ -1677,7 +1676,7 @@
 
 			// Calculate cost
 			cost = curpf->real_cost +
-			       (flags & fpBidiCost ? calc_bidi_cost(cur, *direction) : calc_cost(cur, *direction));
+			       ((flags & fpBidiCost) ? calc_bidi_cost(cur, *direction) : calc_cost(cur, *direction));
 
 			if (neighbpf.cycle != pathfields->cycle) {
 				// add to open list

=== modified file 'src/logic/map_objects/map_object.cc'
--- src/logic/map_objects/map_object.cc	2017-08-09 16:22:02 +0000
+++ src/logic/map_objects/map_object.cc	2017-08-11 11:40:48 +0000
@@ -564,7 +564,7 @@
 			throw wexception("header is %u, expected %u", header, HeaderMapObject);
 
 		uint8_t const packet_version = fr.unsigned_8();
-		if (packet_version <= 0 || packet_version > kCurrentPacketVersionMapObject) {
+		if (packet_version < 1 || packet_version > kCurrentPacketVersionMapObject) {
 			throw UnhandledVersionError("MapObject", packet_version, kCurrentPacketVersionMapObject);
 		}
 

=== modified file 'src/logic/map_objects/tribes/ship.cc'
--- src/logic/map_objects/tribes/ship.cc	2017-07-05 19:21:57 +0000
+++ src/logic/map_objects/tribes/ship.cc	2017-08-11 11:40:48 +0000
@@ -447,7 +447,7 @@
 
 		for (Direction dir = 0; dir <= LAST_DIRECTION; ++dir) {
 			FCoords node = dir ? map.get_neighbour(position, dir) : position;
-			dirs[dir] = node.field->nodecaps() & MOVECAPS_WALK ? 10 : 0;
+			dirs[dir] = (node.field->nodecaps() & MOVECAPS_WALK) ? 10 : 0;
 
 			Area<FCoords> area(node, 0);
 			std::vector<Bob*> ships;

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2017-05-14 20:06:48 +0000
+++ src/logic/save_handler.cc	2017-08-11 11:40:48 +0000
@@ -179,7 +179,6 @@
 			g_fs->fs_rename(complete_filename, backup_filename);
 		}
 
-		std::string error;
 		if (!save_and_handle_error(game, complete_filename, backup_filename)) {
 			return;
 		}

=== modified file 'src/logic/single_player_game_settings_provider.cc'
--- src/logic/single_player_game_settings_provider.cc	2017-02-10 14:12:36 +0000
+++ src/logic/single_player_game_settings_provider.cc	2017-08-11 11:40:48 +0000
@@ -45,7 +45,7 @@
 }
 
 bool SinglePlayerGameSettingsProvider::can_change_player_state(uint8_t number) {
-	return (!s.scenario & (number != s.playernum));
+	return ((!s.scenario) && (number != s.playernum));
 }
 
 bool SinglePlayerGameSettingsProvider::can_change_player_tribe(uint8_t) {

=== modified file 'src/map_io/s2map.cc'
--- src/map_io/s2map.cc	2017-01-25 18:55:59 +0000
+++ src/map_io/s2map.cc	2017-08-11 11:40:48 +0000
@@ -49,7 +49,6 @@
 namespace {
 
 struct S2MapDescrHeader {
-	char magic[10];  // "WORLD_V1.0"
 	char name[20];   // We need fixed char arrays rather than strings here. Otherwise, this will
 	                 // segfault.
 	int16_t w;
@@ -1047,8 +1046,6 @@
 	 * 1: Try to fix port spaces
 	 */
 	const Widelands::Map::PortSpacesSet ports(map_.get_port_spaces());
-	uint16_t num_failed = 0;
-
 	const Widelands::World& world = egbase.world();
 
 	// Check if port spaces are valid
@@ -1074,7 +1071,6 @@
 				}
 			} while (mr.advance(map_) && !fixed);
 			if (!fixed) {
-				++num_failed;
 				log("FAILED! No alternative port buildspace for (%i, %i) found!\n", fc.x, fc.y);
 			} else
 				log("Fixed!\n");

=== modified file 'src/wui/field_overlay_manager.cc'
--- src/wui/field_overlay_manager.cc	2017-01-25 18:55:59 +0000
+++ src/wui/field_overlay_manager.cc	2017-08-11 11:40:48 +0000
@@ -97,16 +97,16 @@
 	Widelands::NodeCaps const caps =
 	   callback_ ? static_cast<Widelands::NodeCaps>(callback_(fc)) : fc.field->nodecaps();
 
-	const int value = caps & Widelands::BUILDCAPS_MINE ?
+	const int value = (caps & Widelands::BUILDCAPS_MINE) ?
 	                     Widelands::Field::Buildhelp_Mine :
 	                     (caps & Widelands::BUILDCAPS_SIZEMASK) == Widelands::BUILDCAPS_BIG ?
-	                     (caps & Widelands::BUILDCAPS_PORT ? Widelands::Field::Buildhelp_Port :
+	                     ((caps & Widelands::BUILDCAPS_PORT) ? Widelands::Field::Buildhelp_Port :
 	                                                         Widelands::Field::Buildhelp_Big) :
 	                     (caps & Widelands::BUILDCAPS_SIZEMASK) == Widelands::BUILDCAPS_MEDIUM ?
 	                     Widelands::Field::Buildhelp_Medium :
 	                     (caps & Widelands::BUILDCAPS_SIZEMASK) == Widelands::BUILDCAPS_SMALL ?
 	                     Widelands::Field::Buildhelp_Small :
-	                     caps & Widelands::BUILDCAPS_FLAG ? Widelands::Field::Buildhelp_Flag :
+	                     (caps & Widelands::BUILDCAPS_FLAG) ? Widelands::Field::Buildhelp_Flag :
 	                                                        Widelands::Field::Buildhelp_None;
 	return value;
 }

=== modified file 'src/wui/game_summary.cc'
--- src/wui/game_summary.cc	2017-06-05 07:33:18 +0000
+++ src/wui/game_summary.cc	2017-08-11 11:40:48 +0000
@@ -134,7 +134,7 @@
 	bool local_in_game = false;
 	bool local_won = false;
 	Widelands::Player* single_won = nullptr;
-	uint8_t teawon_ = 0;
+	Widelands::TeamNumber team_won = 0;
 	InteractivePlayer* ipl = game_.get_ipl();
 	// This defines a row to be selected, current player,
 	// if not then the first line
@@ -171,7 +171,7 @@
 			if (!single_won) {
 				single_won = p;
 			} else {
-				teawon_ = p->team_number();
+				team_won = p->team_number();
 			}
 			break;
 		case Widelands::PlayerEndResult::kResigned:
@@ -196,12 +196,12 @@
 			title_area_->set_text(_("You lost."));
 		}
 	} else {
-		if (teawon_ <= 0) {
+		if (team_won == 0) {
 			assert(single_won);
 			title_area_->set_text((boost::format(_("%s won!")) % single_won->get_name()).str());
 		} else {
 			title_area_->set_text(
-			   (boost::format(_("Team %|1$u| won!")) % static_cast<unsigned int>(teawon_)).str());
+			   (boost::format(_("Team %|1$u| won!")) % static_cast<unsigned int>(team_won)).str());
 		}
 	}
 	if (!players_status.empty()) {

=== modified file 'src/wui/interactive_base.cc'
--- src/wui/interactive_base.cc	2017-07-24 20:35:46 +0000
+++ src/wui/interactive_base.cc	2017-08-11 11:40:48 +0000
@@ -119,7 +119,7 @@
 	sound_subscriber_ = Notifications::subscribe<NoteSound>([this](const NoteSound& note) {
 		if (note.stereo_position != std::numeric_limits<uint32_t>::max()) {
 			g_sound_handler.play_fx(note.fx, note.stereo_position, note.priority);
-		} else if (note.coords != Widelands::Coords(-1, -1)) {
+		} else if (note.coords != Widelands::Coords::null()) {
 			g_sound_handler.play_fx(note.fx, stereo_position(note.coords), note.priority);
 		}
 	});

=== modified file 'src/wui/soldierlist.cc'
--- src/wui/soldierlist.cc	2017-06-24 20:22:19 +0000
+++ src/wui/soldierlist.cc	2017-08-11 11:40:48 +0000
@@ -88,8 +88,8 @@
 		 * so that we can update when its status changes.
 		 */
 		/*@{*/
-		uint32_t cache_level;
-		uint32_t cache_health;
+		uint32_t cache_level = 0;
+		uint32_t cache_health = 0;
 		/*@}*/
 	};
 
@@ -228,9 +228,6 @@
 			icon.pos.x = std::max<int32_t>(icon.pos.x, icon_iter->pos.x + icon_width_);
 		}
 
-		icon.cache_health = 0;
-		icon.cache_level = 0;
-
 		icons_.insert(insertpos, icon);
 		changes = true;
 	}