Address feedback from Clang static analyzer (#1877)

I left alone external libraries, as well as false positives and a bunch of
miscellaneous cases where the existing code is fine (e.g. using floating
point loop indices in scale_surface_sharp() is intentional).

Closes #1877.

src/actions/attack.cpp

@@ -736,7 +736,7 @@ namespace {
 		 * Used in perform_hit to confirm a replay is in sync.
 		 * Check OOS_error_ after this method, true if error detected.
 		 */
-		void check_replay_attack_result(bool, int, int, config, unit_info&);
+		void check_replay_attack_result(bool&, int, int&, config, unit_info&);
 
 		void unit_killed(unit_info &, unit_info &,
 			const battle_context_unit_stats *&, const battle_context_unit_stats *&,
@@ -1333,7 +1333,7 @@ namespace {
 		}
 	}
 
-	void attack::check_replay_attack_result(bool hits, int ran_num, int damage,
+	void attack::check_replay_attack_result(bool& hits, int ran_num, int& damage,
 			config replay_results, unit_info& attacker)
 	{
 		int results_chance = replay_results["chance"];

src/generators/default_map_generator_job.cpp

@@ -553,7 +553,6 @@ static int rank_castle_location(int x, int y, const is_valid_terrain& valid_terr
 		}
 
 		if(distance < min_distance) {
-			avg_distance = 0;
 			return -1;
 		}
 
@@ -966,6 +965,7 @@ std::string default_map_generator_job::default_generate_map(generator_data data,
 		failed_locs.insert(best_loc);
 	}
 
+	LOG_NG << "Placed castles. " << (SDL_GetTicks() - ticks) << " ticks elapsed" << "\n";
 	LOG_NG << "Placing roads...\n";
 	ticks = SDL_GetTicks();
 
@@ -1154,7 +1154,7 @@ std::string default_map_generator_job::default_generate_map(generator_data data,
 		}
 	}
 
-	LOG_NG << "Placed roads and castles. " << (SDL_GetTicks() - ticks) << " ticks elapsed" << "\n";
+	LOG_NG << "Placed roads. " << (SDL_GetTicks() - ticks) << " ticks elapsed" << "\n";
 	ticks = SDL_GetTicks();
 
 	/* Random naming for landforms: mountains, forests, swamps, hills
@@ -1209,6 +1209,7 @@ std::string default_map_generator_job::default_generate_map(generator_data data,
 		}
 	}
 
+	LOG_NG << "Named landforms. " << (SDL_GetTicks() - ticks) << " ticks elapsed" << "\n";
 	LOG_NG << "Placing villages...\n";
 	ticks = SDL_GetTicks();
 

src/gui/dialogs/multiplayer/mp_options_helper.cpp

@@ -65,8 +65,8 @@ void mp_options_helper::update_game_options()
 
 	// For game options, we check for both types and remove them. This is to prevent options from a game
 	// of one type remaining visible when selecting a game of another type.
-	int pos = remove_nodes_for_type("campaign");
-	    pos = remove_nodes_for_type("multiplayer");
+	          remove_nodes_for_type("campaign");
+	int pos = remove_nodes_for_type("multiplayer");
 
 	display_custom_options(type, pos, create_engine_.current_level().data());
 

src/gui/widgets/settings.cpp

@@ -528,6 +528,7 @@ const typename TList::value_type& get_best_resolution(const TList& list, const T
 		}
 	}
 
+	assert(best_resolution != nullptr);
 	return *best_resolution;	
 }
 

src/sdl/utils.cpp

@@ -432,7 +432,7 @@ surface scale_surface_legacy(const surface &surf, int w, int h)
 
 				Uint8 r,g,b,a;
 				Uint32 rr,gg,bb,aa;
-				Uint16 avg_r, avg_g, avg_b, avg_a;
+				Uint16 avg_r, avg_g, avg_b;
 				Uint32 pix[4], bilin[4];
 
 				// This next part is the fixed point
@@ -466,7 +466,7 @@ surface scale_surface_legacy(const surface &surf, int w, int h)
 				// what the pixel values are like.
 
 				int count = 0;
-				avg_r = avg_g = avg_b = avg_a = 0;
+				avg_r = avg_g = avg_b = 0;
 				int loc;
 				for (loc=0; loc<4; loc++) {
 				  a = pix[loc] >> 24;
@@ -477,7 +477,6 @@ surface scale_surface_legacy(const surface &surf, int w, int h)
 				    avg_r += r;
 				    avg_g += g;
 				    avg_b += b;
-				    avg_a += a;
 				    count++;
 				  }
 				}
@@ -485,7 +484,6 @@ surface scale_surface_legacy(const surface &surf, int w, int h)
 				  avg_r /= count;
 				  avg_b /= count;
 				  avg_g /= count;
-				  avg_a /= count;
 				}
 
 				// Perform modified bilinear interpolation.

src/serialization/preprocessor.cpp

@@ -195,6 +195,8 @@ class preprocessor
 	std::string old_textdomain_;
 	std::string old_location_;
 	int old_linenum_;
+
+	friend class preprocessor_streambuf;
 protected:
 	preprocessor_streambuf &target_;
 	preprocessor(preprocessor_streambuf &);
@@ -207,7 +209,7 @@ public:
 	virtual bool get_chunk() = 0;
 	///retruns 1 if this parses a macro, -1 if this doesnt parse text (if this is no preprocessor_data). 0 otherwise (this parses a file).
 	virtual int is_macro() { return -1; }
-	virtual ~preprocessor();
+	virtual ~preprocessor() {}
 };
 
 /**
@@ -218,6 +220,7 @@ class preprocessor_streambuf: public streambuf
 {
 	std::string out_buffer_;      /**< Buffer read by the STL stream. */
 	virtual int underflow();
+	void restore_old_preprocessor(const preprocessor& current);
 	std::stringstream buffer_;    /**< Buffer filled by the #current_ preprocessor. */
 	preprocessor *current_;       /**< Input preprocessor. */
 	preproc_map *defines_;
@@ -330,7 +333,9 @@ int preprocessor_streambuf::underflow()
 		// Process files and data chunks until the desired buffer size is reached
 		if (!current_->get_chunk()) {
 			 // Delete the current preprocessor item to restore its predecessor
-			delete current_;
+			preprocessor* temp = current_;
+			restore_old_preprocessor(*temp);
+			delete temp;
 		}
 	}
 	// Update the internal state and data pointers
@@ -346,6 +351,26 @@ int preprocessor_streambuf::underflow()
 	return static_cast<unsigned char>(*(begin + sz));
 }
 
+/**
+* Restores the old preprocessing context.
+* Appends location and domain directives to the buffer, so that the parser
+* notices these changes.
+*/
+void preprocessor_streambuf::restore_old_preprocessor(const preprocessor& current)
+{
+	if(!current.old_location_.empty()) {
+		buffer_ << "\376line " << current.old_linenum_ << ' ' << current.old_location_ << '\n';
+	}
+	if(!current.old_textdomain_.empty() && textdomain_ != current.old_textdomain_) {
+		buffer_ << "\376textdomain " << current.old_textdomain_ << '\n';
+	}
+	current_ = current.old_preprocessor_;
+	location_ = current.old_location_;
+	linenum_ = current.old_linenum_;
+	textdomain_ = current.old_textdomain_;
+	--depth_;
+}
+
 std::string lineno_string(const std::string &lineno)
 {
 	std::vector< std::string > pos = utils::quoted_split(lineno, ' ');
@@ -407,27 +432,6 @@ preprocessor::preprocessor(preprocessor_streambuf &t) :
 	target_.current_ = this;
 }
 
-/**
- * Restores the old preprocessing context of #target_.
- * Appends location and domain directives to the buffer, so that the parser
- * notices these changes.
- */
-preprocessor::~preprocessor()
-{
-	assert(target_.current_ == this);
-	if (!old_location_.empty()) {
-		target_.buffer_ << "\376line " << old_linenum_ << ' ' << old_location_ << '\n';
-	}
-	if (!old_textdomain_.empty() && target_.textdomain_ != old_textdomain_) {
-		target_.buffer_ << "\376textdomain " << old_textdomain_ << '\n';
-	}
-	target_.current_  = old_preprocessor_;
-	target_.location_ = old_location_;
-	target_.linenum_  = old_linenum_;
-	target_.textdomain_ = old_textdomain_;
-	--target_.depth_;
-}
-
 /**
  * Specialized preprocessor for handling a file or a set of files.
  * A preprocessor_file object is created when a preprocessor encounters an

src/terrain/builder.cpp

@@ -164,6 +164,8 @@ void terrain_builder::tile::rebuild_cache(const std::string& tod, logs* log)
 
 			img_list.push_back(anim);
 
+			assert(anim.get_animation_duration() != 0);
+
 			if(variant.random_start)
 				img_list.back().set_animation_time(ri.rand % img_list.back().get_animation_duration());
 
@@ -1183,6 +1185,8 @@ void terrain_builder::build_terrains()
 			}
 		}
 
+		assert(min_constraint != nullptr);
+
 		// NOTE: if min_types is not empty, we have found a valid min_constraint;
 		for(t_translation::ter_list::const_iterator t = min_types.begin(); t != min_types.end(); ++t) {
 			const std::vector<map_location>* locations = &terrain_by_type_[*t];

src/terrain/translation.cpp

@@ -736,14 +736,12 @@ static terrain_code string_to_number_(const std::string& str, const ter_layer fi
 static terrain_code string_to_number_(std::string str, std::string& start_position, const ter_layer filler)
 {
 	const char* c_str = str.c_str();
-	size_t begin = 0;
-	size_t end = str.size();
 	terrain_code result;
 
 	// Strip the spaces around us
 	const std::string& whitespace = " \t";
-	begin = str.find_first_not_of(whitespace);
-	end = str.find_last_not_of(whitespace) + 1;
+	size_t begin = str.find_first_not_of(whitespace);
+	size_t end = str.find_last_not_of(whitespace) + 1;
 	if(begin == std::string::npos) {
 		return result;
 	}

src/widgets/scrollbar.cpp

@@ -348,7 +348,7 @@ void scrollbar::handle_event(const SDL_Event& event)
 				move_position(-static_cast<int>(grip_height_));
 			else
 				move_position(grip_height_);
-		} else if (on_groove && e.button == SDL_BUTTON_MIDDLE) {
+		} else if (on_groove && e.button == SDL_BUTTON_MIDDLE && groove.h != grip.h) {
 			int y_dep = e.y - grip.y - grip.h/2;
 			int dep = y_dep * int(full_height_ - grip_height_)/ (groove.h - grip.h);
 			move_position(dep);