See diff comments.

Diff comments:

> 
> === modified file 'src/scripting/lua_map.cc'
> --- src/scripting/lua_map.cc  2016-04-11 06:45:29 +0000
> +++ src/scripting/lua_map.cc  2016-04-16 12:42:55 +0000
> @@ -1871,10 +1871,16 @@
>  /* RST
>       .. attribute:: workarea_radius
>  
> -                     (RO) the workarea_radius of the building as an int.
> +                     (RO) the first workarea_radius of the building as an 
> int, 
> +                 0 in case bulding has no workareas
>  */
>  int LuaBuildingDescription::get_workarea_radius(lua_State * L) {
> -     lua_pushinteger(L, get()->workarea_info_.begin()->first);
> +    const WorkareaInfo& workareaInfo = get()->workarea_info_;

Please use tabs for indentation.

> +    if (!workareaInfo.empty()) {
> +        lua_pushinteger(L, workareaInfo.begin()->first);
> +    } else {
> +        lua_pushinteger(L, 0);

I think it would be safer to use lua_pushnil here to indicate that there is NO 
work area defined. Returning 0 could be understood as a work area with 0 
radius, even if it may not make much sense. This behavior is also consistent 
with many of our other functions. You don't have to modify any Lua code because 
it already checks for a nil value.

> +    }
>       return 1;
>  }
>  


-- 
https://code.launchpad.net/~widelands-dev/widelands/bug_1571009_work_area_radius/+merge/292066
Your team Widelands Developers is requested to review the proposed merge of 
lp:~widelands-dev/widelands/bug_1571009_work_area_radius into lp:widelands.

_______________________________________________
Mailing list: https://launchpad.net/~widelands-dev
Post to     : widelands-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~widelands-dev
More help   : https://help.launchpad.net/ListHelp

Reply via email to