widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

So ... you created a diff of 800 lines by passing objects around so you can call a method in only a handful of places?? I like it! :)
The changes are mostly looking good to me. Replacing the GOTO might be broken, though, see my diff comment below.

I haven't tested the changes, mostly because I have no sensible idea how to do so. But based on the code I guess it shouldn't break anything.

Diff comments:

> 
> === modified file 'src/map_io/map_buildingdata_packet.cc'
> --- src/map_io/map_buildingdata_packet.cc	2019-05-18 11:58:43 +0000
> +++ src/map_io/map_buildingdata_packet.cc	2019-05-26 03:34:22 +0000
> @@ -615,14 +618,15 @@
>  					if (worker_descr.can_act_as(working_position.first)) {
>  						while (wp->worker || wp->worker_request) {
>  							++wp;
> -							if (!--count)
> -								goto end_working_position;
> +							if (!--count) {
> +								continue;
> +							}

While I appreciate getting rid of GOTO, I am not sure if the logic change here is intentional. I think the continue will influence the while() loop while I guess that you want to influence the for() loop?
A possible replacement would be setting a flag and break-ing within the while(), then afterwards check for the flag, resetting it and using continue.

>  						}
>  						found_working_position = true;
>  						break;
> -					} else
> +					} else {
>  						wp += count;
> -				end_working_position:;
> +					}
>  				}
>  
>  				if (!found_working_position)
> 
> === modified file 'src/map_io/map_flagdata_packet.h'
> --- src/map_io/map_flagdata_packet.h	2019-02-23 11:00:49 +0000
> +++ src/map_io/map_flagdata_packet.h	2019-05-26 03:34:22 +0000
> @@ -21,7 +21,14 @@
>  #define WL_MAP_IO_MAP_FLAGDATA_PACKET_H
>  
>  #include "map_io/map_data_packet.h"
> +#include "map_io/tribes_legacy_lookup_table.h"
>  
> -MAP_DATA_PACKET(MapFlagdataPacket)
> +namespace Widelands {
> +class MapFlagdataPacket {
> +public:
> +	void read(FileSystem&, EditorGameBase&, bool, MapObjectLoader&, const TribesLegacyLookupTable& tribes_lookup_table);
> +	void write(FileSystem&, EditorGameBase&, MapObjectSaver&);
> +};
> +}

Maybe provide a second macro for the extended version? Just an idea, this way is okay as well.

>  
>  #endif  // end of include guard: WL_MAP_IO_MAP_FLAGDATA_PACKET_H


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