widelands-dev team mailing list archive

[Klaus Halfmann <klaus.halfmann@xxxxxxxx>]

Review: Approve

Found only intended renamings or improvements,
Added some comments about minor things.

New for me are warnings like:
[ 44%] Building CXX object src/logic/CMakeFiles/logic.dir/map_objects/tribes/requirements.cc.o
.../bug-1395278-editor/src/logic/map_objects/tribes/requirements.cc:247:5: warning: format specifies type 'unsigned int' but the argument has underlying type 'uint8_t' (aka 'unsigned char') [-Wformat] TrainingAttribute::kHealth,

I think someone reenabled that clang warning.

Can be merged


Diff comments:

> 
> === modified file 'src/editor/tools/editor_increase_height_tool.h'
> --- src/editor/tools/editor_increase_height_tool.h	2016-01-28 05:24:34 +0000
> +++ src/editor/tools/editor_increase_height_tool.h	2016-03-18 12:52:47 +0000
> @@ -30,8 +30,8 @@
>  	 EditorSetHeightTool    &   the_set_tool)
>  		:
>  		EditorTool(the_decrease_tool, the_set_tool),
> -		m_decrease_tool(the_decrease_tool), m_set_tool(the_set_tool),
> -		m_change_by(1)
> +		decrease_tool_(the_decrease_tool), set_tool_(the_set_tool),
> +		change_by_(1)

Why use the_ as a prefix, strange?

>  	{}
>  
>  	int32_t handle_click_impl(const Widelands::World& world,
> 
> === modified file 'src/editor/tools/editor_tool.h'
> --- src/editor/tools/editor_tool.h	2016-02-01 08:21:18 +0000
> +++ src/editor/tools/editor_tool.h	2016-03-18 12:52:47 +0000
> @@ -62,23 +62,23 @@
>  		EditorInteractive& parent, EditorActionArgs* args, Widelands::Map* map)
>  	{
>  		return
> -		    (i == First ? *this : i == Second ? m_second : m_third)
> +			 (i == First ? *this : i == Second ? second_ : third_)
>  		    .handle_undo_impl(world, center, parent, args, map);
>  	}
>  
>  	const char * get_sel(const ToolIndex i) {
>  		return
> -		    (i == First ? *this : i == Second ? m_second : m_third)
> +			 (i == First ? *this : i == Second ? second_ : third_)
>  		    .get_sel_impl();
>  	}
>  
>  	EditorActionArgs format_args(const ToolIndex i, EditorInteractive & parent) {
>  		return
> -		    (i == First ? *this : i == Second ? m_second : m_third)
> +			 (i == First ? *this : i == Second ? second_ : third_)

This (i == First ? *this : i == Second ? second_ : third_) would benefit from an inline function

>  		    .format_args_impl(parent);
>  	}
>  
> -	bool is_unduable() {return undoable;}
> +	bool is_undoable() {return undoable_;}
>  	virtual bool has_size_one() const {return false;}
>  	virtual EditorActionArgs format_args_impl(EditorInteractive & parent) {
>  		return EditorActionArgs(parent);


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug-1395278-editor/+merge/289494
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/bug-1395278-editor.