widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Review: Approve review/compile

Moste of the code is correct, we mave some "Upgrades" form 8bit to 32bit,
that where broken before but got unnoticed, but now they will become visible.
Please check my inline comments

Id really like to have some coverage tool that checks that this code is actually
tested. Still OK for me.

Will run some test now and try to play a game perhpas today.

Diff comments:

> 
> === modified file 'src/editor/map_generator.cc'
> --- src/editor/map_generator.cc	2016-04-06 09:23:04 +0000
> +++ src/editor/map_generator.cc	2016-04-11 07:07:30 +0000
> @@ -130,9 +130,9 @@
>  	const auto set_resource_helper = [this, &world, &terrain_description, &fc] (
>  	   const uint32_t random_value, const int valid_resource_index) {
>  		const DescriptionIndex  res_idx = terrain_description.get_valid_resource(valid_resource_index);
> -		const uint32_t max_amount = world.get_resource(res_idx)->max_amount();
> -		uint8_t res_val = static_cast<uint8_t>(random_value / (kMaxElevation / max_amount));
> -		res_val *= static_cast<uint8_t>(map_info_.resource_amount) + 1;
> +		const ResourceAmount max_amount = world.get_resource(res_idx)->max_amount();
> +		ResourceAmount res_val = static_cast<ResourceAmount>(random_value / (kMaxElevation / max_amount));
> +		res_val *= static_cast<ResourceAmount>(map_info_.resource_amount) + 1;

Values of this calucation may have changed as the cast to uint8_t may have limited the value to 256,
I took a closer look but got not clue what this is finally about, mmh.

>  		res_val /= 3;
>  		if (map_.is_resource_valid(world, fc, res_idx)) {
>  			map_.initialize_resources(fc, res_idx, res_val);
> 
> === modified file 'src/editor/tools/set_resources_tool.cc'
> --- src/editor/tools/set_resources_tool.cc	2016-04-06 09:23:04 +0000
> +++ src/editor/tools/set_resources_tool.cc	2016-04-11 07:07:30 +0000
> @@ -68,11 +68,9 @@
>                                           EditorActionArgs* args,
>                                           Widelands::Map* map) {
>  	for (const auto & res : args->original_resource) {
> -		int32_t amount     = res.amount;
> -		int32_t max_amount = world.get_resource(args->current_resource)->max_amount();
> +		Widelands::ResourceAmount amount     = res.amount;
> +		Widelands::ResourceAmount max_amount = world.get_resource(args->current_resource)->max_amount();
>  
> -		if (amount < 0)
> -			amount = 0;

Mhh, instead of protection against underflow, we now dow this we the next statement.

>  		if (amount > max_amount)
>  			amount = max_amount;
>  
> 
> === modified file 'src/logic/map.cc'
> --- src/logic/map.cc	2016-04-06 09:23:04 +0000
> +++ src/logic/map.cc	2016-04-11 07:07:30 +0000
> @@ -1869,7 +1869,7 @@
>  }
>  
>  bool Map::is_resource_valid
> -	(const Widelands::World& world, const TCoords<Widelands::FCoords>& c, int32_t const curres)
> +	(const Widelands::World& world, const TCoords<Widelands::FCoords>& c, DescriptionIndex curres)

a const was lost here?

>  {
>  	if (curres == Widelands::kNoResource)
>  		return true;


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1545243-plnum-lua/+merge/291481
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1545243-plnum-lua.