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.
_______________________________________________
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
