widelands-dev team mailing list archive

[Arty <diego.scheide@xxxxxx>]

Code looks good in principle, but haven't tested yet whether it really works as intended.

Two minor nits:
1) see diff comment
2) All the new lines (except when they were copied over) are indented with spaces instead of tabs.


Diff comments:

> === modified file 'src/logic/map_objects/tribes/building.cc'
> --- src/logic/map_objects/tribes/building.cc	2018-09-23 11:10:56 +0000
> +++ src/logic/map_objects/tribes/building.cc	2018-11-16 06:07:58 +0000
> @@ -151,9 +156,9 @@
>  			return_enhanced_ =
>  			   Buildcost(table.get_table("return_on_dismantle_on_enhanced"), egbase_.tribes());
>  		} catch (const WException& e) {
> -			throw wexception("An enhanced building must define \"enhancement_cost\""
> +			throw wexception("The enhanced building '%s' must define \"enhancement_cost\""

This message is (and was before) not really related to this catch. If this catch block is reached then either because the other key ("return_on_dismantle_on_enhanced") wasn't defined or because the Buildcost constructor threw when trying to read one of the table.

I think you should just get rid of the whole try-catch here and do it the same way you already did in the "buildcost" part above, i.e., check if the second key ("return_on_dismantle_on_enhanced") is defined and throw if not, but don't actually catch any exceptions. The Buildcost constructor will throw with a proper message anyway if it fails, no need to catch that and throw again with a message that is possibly even unrelated.

>  			                 "and \"return_on_dismantle_on_enhanced\": %s",
> -			                 e.what());
> +			                 name().c_str(), e.what());
>  		}
>  	}
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/empire04_unused_key_return_on_dismantle_no_ui/+merge/358305
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/empire04_unused_key_return_on_dismantle_no_ui.