Replace boost::foreach and container iterate with range based for loops

[SirVer]

I wanted to work on this right now, but realized that range based for are only supported with gcc >= 4.6, so it seems a little early to jump ship here. After b18 we should reconsider doing this move though.

[SirVer (sirver)]

all BOOST_FOREACH are gone for a while and there are rules for cmake/codecheck/rules/do_not_use_BOOST_FOREACH . There are very many (const_)container_iterates in the code base still though that should be replaced.

[GunChleoc (gunchleoc)]

I found a code example in a recent merge request, so I can take this one on

18 - container_iterate_const(std::vector<TribeBasicInfo>, s.tribes, i)
19 - if (i.current->name == player.tribe) {
20 + for (const TribeBasicInfo tmp_tribe : s.tribes)
21 + {
22 + if (tmp_tribe.name == player.tribe) {
23 s.players[number].tribe = actual_tribe;
24 - if (i.current->initializations.size() <= player.initialization_index) {
25 + if (tmp_tribe.initializations.size() <= player.initialization_index) {
26 player.initialization_index = 0;
27 }
28 }
29 + }

[SirVer (sirver)]

Make a few first and send for review so that we can iron out style questions before we need to retouch hundreds of code lines later.

> Am 16.07.2014 um 22:09 schrieb GunChleoc <email address hidden>:
>
> I found a code example in a recent merge request, so I can take this one
> on
>
> 18 - container_iterate_const(std::vector<TribeBasicInfo>, s.tribes, i)
> 19 - if (i.current->name == player.tribe) {
> 20 + for (const TribeBasicInfo tmp_tribe : s.tribes)
> 21 + {
> 22 + if (tmp_tribe.name == player.tribe) {
> 23 s.players[number].tribe = actual_tribe;
> 24 - if (i.current->initializations.size() <= player.initialization_index) {
> 25 + if (tmp_tribe.initializations.size() <= player.initialization_index) {
> 26 player.initialization_index = 0;
> 27 }
> 28 }
> 29 + }
>
> ** Changed in: widelands
> Assignee: (unassigned) => GunChleoc (gunchleoc)
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1203629
>
> Title:
> Replace boost::foreach and container iterate with range based for
> loops
>
> Status in Widelands:
> Confirmed
>
> Bug description:
> I wanted to work on this right now, but realized that range based for
> are only supported with gcc >= 4.6, so it seems a little early to jump
> ship here. After b18 we should reconsider doing this move though.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/widelands/+bug/1203629/+subscriptions

[GunChleoc (gunchleoc)]

I tried one and it doesn't work. Could somebody please have a look and fix up my last commit?

[SirVer (sirver)]

Your code compiles for me. What is the error?

I added a small comment/nit in r7108.

[GunChleoc (gunchleoc)]

Your nit fixed the problem

[GunChleoc (gunchleoc)]

I need help with the pointer thing. In worker.cc, I have a working implementation, looks like this:

  for (const ImmovableFound& tmp_flag : flags)
  {
   Flag & flag = ref_cast<Flag, BaseImmovable>(*tmp_flag.object);

   if (game.logic_rand() % 2 == 0)
    continue;

   int32_t const dist =
    map.calc_distance(get_position(), tmp_flag.coords);

"flags" is of the sollowing type:

std::vector<ImmovableFound> flags;

And ImmovableFound is:

struct ImmovableFound {
 BaseImmovable * object;
 Coords coords;
};

Now, I'm rtying to do the same thing in flag.cc. My attempts compile, but I get an error when I start a game and place a flag.

Attempt1:
 for (const FlagJob& tmp_flagjobs: m_flag_jobs)
  *tmp_flagjobs.request->set_economy(e);

Attempt2:

 for (const FlagJob tmp_flagjobs: m_flag_jobs)
  tmp_flagjobs.request->set_economy(e);

and the types are:

typedef std::list<FlagJob> FlagJobs;
 FlagJobs m_flag_jobs;

 struct FlagJob {
  Request * request;
  std::string program;
 };

[SirVer (sirver)]

this should work:

for (FlagJob& tmp_flagjobs: m_flag_jobs)
  tmp_flagjobs.request->set_economy(e);

no need to dereference tmp_flagjobs in your first example. And I think const does not work here as ->set_economy() cannot work on a const Request.

[GunChleoc (gunchleoc)]

So, I leave those who call some function that changes stuff alone for now?

[SirVer (sirver)]

No, why? You just need to take a non-constant reference in your loop variable. Is there a particular case that gives you trouble?

> Am 18.07.2014 um 09:00 schrieb GunChleoc <email address hidden>:
>
> So, I leave those who call some function that changes stuff alone for
> now?
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1203629
>
> Title:
> Replace boost::foreach and container iterate with range based for
> loops
>
> Status in Widelands:
> Confirmed
>
> Bug description:
> I wanted to work on this right now, but realized that range based for
> are only supported with gcc >= 4.6, so it seems a little early to jump
> ship here. After b18 we should reconsider doing this move though.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/widelands/+bug/1203629/+subscriptions

[GunChleoc (gunchleoc)]

Not really, just trying to figure out how I can recognize when I'm dealing with a pointer, when I'm dealing with a reference, and when I need a const or not. The compiler doesn't tell me anything, so it will only break during testing if I made a mistake. Flags are easy to test, other stuff might come around and bit me in the ass. I haven't looked at this today (working on another bug), but I might still find enlightenment *lol

[GunChleoc (gunchleoc)]

Actually, the last commit did not fix it as I thought. The flag and road do get created now, but as soon as a carrier leaves the headquarters, the road and flag get destroyed.

I think I need somebody to do a couple of these for me so I can get some code samples to analyse and try to figure out what is happening and why, because I haven't a clue.

[SirVer (sirver)]

We can pair on this for an hour or two. I shoot you a mail.

[GunChleoc (gunchleoc)]

Fixed in build19-rc1.