widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Review: Approve compile, review

Looks good to me, with Apple clang I get a lot less warnings.
You removed on break; that looks incorrect, but I am but sure,
see diff comments.

I prepared some changes that would remove some implicit float -> double warnings, too.
Not sure if I should commit these, though.

Diff comments:

> 
> === modified file 'src/ai/ai_help_structs.h'
> --- src/ai/ai_help_structs.h	2016-03-12 20:06:24 +0000
> +++ src/ai/ai_help_structs.h	2016-04-02 16:49:56 +0000
> @@ -351,8 +350,8 @@
>  	uint16_t mines_percent;  // % of res it can mine
>  	uint32_t current_stats;
>  
> -	std::vector<int16_t> inputs;
> -	std::vector<int16_t> outputs;
> +	std::vector<Widelands::DescriptionIndex> inputs;
> +	std::vector<Widelands::DescriptionIndex> outputs;

What _is_ a DescriptionIndex? Found it in src/logic/widelands.h but where is the object this index is ued for?

>  	std::vector<Widelands::DescriptionIndex> critical_building_material;
>  
>  	bool produces_building_material;
> 
> === modified file 'src/editor/ui_menus/editor_tool_change_resources_options_menu.cc'
> --- src/editor/ui_menus/editor_tool_change_resources_options_menu.cc	2016-03-14 18:10:09 +0000
> +++ src/editor/ui_menus/editor_tool_change_resources_options_menu.cc	2016-04-02 16:49:56 +0000
> @@ -197,10 +197,7 @@
>  	case Change_By_Increase: change_by += change_by < 63; break;
>  	case Change_By_Decrease: change_by -= 1 < change_by;  break;
>  	case    Set_To_Increase: set_to    += set_to    < 63; break;
> -	case    Set_To_Decrease: set_to    -= 0 < set_to;     break;
> -	default:
> -		NEVER_HERE();
> -		break;
> +	case    Set_To_Decrease: set_to    -= 0 < set_to;

-=0 // thats a noop?

>  	}
>  	increase_tool_.set_change_by(change_by);
>  	increase_tool_.decrease_tool().set_change_by(change_by);
> 
> === modified file 'src/logic/map_objects/bob.cc'
> --- src/logic/map_objects/bob.cc	2016-03-31 06:56:28 +0000
> +++ src/logic/map_objects/bob.cc	2016-04-02 16:49:56 +0000
> @@ -712,7 +712,7 @@
>  	// Slowing down a ship if two or more on same spot
>  	// Using probability of 1/8 and pausing it for 5, 10 or 15 seconds
>  	if (game.logic_rand() % 8 == 0) {
> -		if (upcast(Ship, ship, this)) {
> +		if (is_a(Ship, this)) {

Ah, much better!

>  			Map& map = game.map();
>  			const uint32_t ships_count
>  				= map.find_bobs(Widelands::Area<Widelands::FCoords>(get_position(), 0), nullptr, FindBobShip());
> 
> === modified file 'src/scripting/lua_globals.cc'
> --- src/scripting/lua_globals.cc	2015-02-28 17:43:48 +0000
> +++ src/scripting/lua_globals.cc	2016-04-02 16:49:56 +0000
> @@ -104,7 +104,6 @@
>  				case LUA_TTHREAD:
>  				case LUA_TLIGHTUSERDATA:
>  					report_error(L, "Cannot format the given type %s at index %i", lua_typename(L, i), i);
> -					break;

This looks wrong, perhaps:
NEVER_HERE(); // as report_error will never return
would be better?

>  
>  				default:
>  					{
> 
> === modified file 'src/ui_basic/multilinetextarea.h'
> --- src/ui_basic/multilinetextarea.h	2016-03-25 17:01:05 +0000
> +++ src/ui_basic/multilinetextarea.h	2016-04-02 16:49:56 +0000
> @@ -74,7 +74,7 @@
>  
>  	/**
>  	 * This prepares a non-richtext text for rendering. It escapes the source text and
> -	 * turns \n into <br> tags as needed, then creates the richtext style wrappers.
> +	 * turns '\\n' into '<br>' tags as needed, then creates the richtext style wrappers.

Thx, that was the most annyoing one

>  	 */
>  	std::string make_richtext();
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/travis-clang-warnings/+merge/290697
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/travis-clang-warnings.