widelands-dev team mailing list archive

[SirVer <SirVer@xxxxxx>]

Review: Needs Fixing



Diff comments:

> 
> === modified file 'src/base/md5.h'
> --- src/base/md5.h	2017-01-25 18:55:59 +0000
> +++ src/base/md5.h	2017-06-28 08:25:35 +0000
> @@ -74,7 +74,7 @@
>   */
>  template <typename Base> class MD5Checksum : public Base {
>  public:
> -	MD5Checksum() {
> +	MD5Checksum() : Base() {

unnecessary, the default constructor will always be called.

>  		Reset();
>  	}
>  	explicit MD5Checksum(const MD5Checksum& other)
> 
> === modified file 'src/economy/shippingitem.h'
> --- src/economy/shippingitem.h	2017-06-24 08:47:46 +0000
> +++ src/economy/shippingitem.h	2017-06-28 08:25:35 +0000
> @@ -59,6 +59,8 @@
>  	void remove(EditorGameBase&);
>  
>  	struct Loader {
> +		Loader () : serial_(0U) {

c++11 allows to initial PODs inline. I think that is preferable to adding constructors, i.e. just write 

int serial_ = 0; 

below.

> +		}
>  		void load(FileRead& fr);
>  		ShippingItem get(MapObjectLoader& mol);
>  
> 
> === modified file 'src/game_io/game_preload_packet.cc'
> --- src/game_io/game_preload_packet.cc	2017-01-25 18:55:59 +0000
> +++ src/game_io/game_preload_packet.cc	2017-06-28 08:25:35 +0000
> @@ -46,6 +46,9 @@
>  constexpr uint16_t kCurrentPacketVersion = 6;
>  constexpr const char* kMinimapFilename = "minimap.png";
>  
> +GamePreloadPacket::GamePreloadPacket() : minimap_path_(""), mapname_(""), background_(""), win_condition_(""), gametime_(0), player_nr_(Widelands::neutral()), version_(""), savetimestamp_(0), gametype_(GameController::GameType::kUndefined) {

unnecessary to initially std::string, it will always initialized to be empty. inline initialize the PODs in the header, should also work for the enum.

> +}
> +
>  std::string GamePreloadPacket::get_localized_win_condition() const {
>  	i18n::Textdomain td("win_conditions");
>  	return _(win_condition_);
> 
> === modified file 'src/graphic/gl/fields_to_draw.h'
> --- src/graphic/gl/fields_to_draw.h	2017-05-13 18:48:26 +0000
> +++ src/graphic/gl/fields_to_draw.h	2017-06-28 08:25:35 +0000
> @@ -78,6 +78,7 @@
>  	};
>  
>  	FieldsToDraw() {
> +		reset(0, 0, 0, 0);

This should always be reset before used anyways. How about adding a comment that this is to appease cppcheck?

>  	}
>  
>  	// Resize this fields to draw for reuse.
> 
> === modified file 'src/graphic/graphic.cc'
> --- src/graphic/graphic.cc	2017-05-31 21:27:07 +0000
> +++ src/graphic/graphic.cc	2017-06-28 08:25:35 +0000
> @@ -59,7 +59,7 @@
>  
>  }  // namespace
>  
> -Graphic::Graphic() : image_cache_(new ImageCache()), animation_manager_(new AnimationManager()) {
> +Graphic::Graphic() : window_mode_width_(0), window_mode_height_(0), sdl_window_(nullptr), max_texture_size_(kMinimumSizeForTextures), image_cache_(new ImageCache()), animation_manager_(new AnimationManager()) {

inline initialize POD

>  }
>  
>  /**
> 
> === modified file 'src/graphic/render_queue.h'
> --- src/graphic/render_queue.h	2017-01-25 18:55:59 +0000
> +++ src/graphic/render_queue.h	2017-06-28 08:25:35 +0000
> @@ -113,7 +113,7 @@
>  	};
>  
>  	struct TerrainArguments {
> -		TerrainArguments() {
> +		TerrainArguments() : gametime(0), renderbuffer_width(0), renderbuffer_height(0), fields_to_draw(nullptr), scale(1.f) {

here too and below. will not repeat every time.

>  		}
>  
>  		int gametime;
> 
> === modified file 'src/logic/field.cc'
> --- src/logic/field.cc	2017-01-25 18:55:59 +0000
> +++ src/logic/field.cc	2017-06-28 08:25:35 +0000
> @@ -23,6 +23,9 @@
>  
>  namespace Widelands {
>  
> +Field::Field() : bobs(nullptr), immovable(nullptr) , caps(7), roads(6), height(0), brightness(0), owner_info_and_selections(Widelands::neutral()), resources(INVALID_INDEX), initial_res_amount(0), res_amount(0), terrains{INVALID_INDEX, INVALID_INDEX} {

I guess this was intentionally left uninitialized because we make many of these fields and will never use the default constructed value anyways. I doubt that we care about that performance anymore, since it will only be done once when the game is started. I suggest inline initializing this as much as possible too.

> (Seems like someone was trying to save some memory there. Not sure whether this try is successful.)

There is a static_assert to check that it is: static_assert(sizeof(Field) == sizeof(void*) * 2 + 10, "Field is not tightly packed."); so on 64bit architecture, Field is 8*2+10 = 26 bytes. On a 512x512 map this amounts to ~6MB. I guess the bit fiddling reaches back to a time where this was a lot. 

Player::Field does not use this bid fiddling, and IMHO every Player has a full copy of all fields, so for 8 players we are ~50MB only for the maps.

After this analysis I agree that the bid fiddling probably does not make a lot of sense in this day and age anymore.

> +}
> +
>  /**
>   * Set the field's brightness based upon the slopes.
>   * Slopes are calulated as this field's height - neighbour's height.


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables.