widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Thanks for your efforts on cleaning up the code!

Most of this branch looks good to me, although I have a a few comments. The first two points should probably be fixed, the other ones are not so important.

Note that I haven't tried compiling or testing, I only looked at the code.

Diff comments:

> 
> === 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);

With this we end up with w_=h_=1 and field_.size() of 1. I think the values of w_ and h_ are fine (checks are done for <w_) but a size() of 0 would be preferable. Haven't checked whether anyone cares about the size, though.

>  	}
>  
>  	// Resize this fields to draw for reuse.
> 
> === 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} {

Where are the values 7 and 6 are coming from? As far as I know, the ": 7" below is not an initialization but a hint to the compiler, how many bits this variable uses.
(Seems like someone was trying to save some memory there. Not sure whether this try is successful.)

> +}
> +
>  /**
>   * Set the field's brightness based upon the slopes.
>   * Slopes are calulated as this field's height - neighbour's height.
> 
> === modified file 'src/logic/map_objects/tribes/militarysite.h'
> --- src/logic/map_objects/tribes/militarysite.h	2017-06-25 19:12:30 +0000
> +++ src/logic/map_objects/tribes/militarysite.h	2017-06-28 08:25:35 +0000
> @@ -34,6 +34,13 @@
>  class Soldier;
>  class World;
>  
> +// I assume elsewhere, that enum SoldierPreference fits to uint8_t.
> +enum class SoldierPreference : uint8_t {
> +	kNotSet,

I think we only need kNotSet for a few seconds when loading a savegame. Maybe just drop it?

> +	kRookies,
> +	kHeroes,
> +};
> +
>  class MilitarySiteDescr : public BuildingDescr {
>  public:
>  	MilitarySiteDescr(const std::string& init_descname,
> 
> === modified file 'src/logic/map_objects/world/map_gen.cc'
> --- src/logic/map_objects/world/map_gen.cc	2017-01-25 18:55:59 +0000
> +++ src/logic/map_objects/world/map_gen.cc	2017-06-28 08:25:35 +0000
> @@ -84,7 +84,7 @@
>  
>  MapGenAreaInfo::MapGenAreaInfo(const LuaTable& table,
>                                 const World& world,
> -                               MapGenAreaType const areaType) {
> +                               MapGenAreaType const area_type) {

Why not also rename the parameter in the other methods?

>  	weight_ = get_positive_int(table, "weight");
>  
>  	const auto read_terrains = [this, &table, &world](
> 
> === modified file 'src/wui/debugconsole.cc'
> --- src/wui/debugconsole.cc	2017-01-25 18:55:59 +0000
> +++ src/wui/debugconsole.cc	2017-06-28 08:25:35 +0000
> @@ -90,6 +91,7 @@
>  		ChatMessage cm;
>  
>  		cm.time = time(nullptr);
> +		cm.playern = Widelands::neutral();

Maybe create a constructor for chat messages which only takes a message?

>  		cm.msg = msg;
>  		messages.push_back(cm);
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables/+merge/326256
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/bug-986611-cppcheck-uninitialized-variables into lp:widelands.