std::for_each is the way to go.
Granted, though it would not address the fact that as well as broken the code is naff in terms of efficiency.
But std::for_each is a bit extra work - there is always going to be the temptation to write a "quick" for loop.
And no raw pointers unless you have to.
Agree, mostly, there are non here, as it happens.
Every language will have bad code written in it.
The thing is that it isn't terribly obvious that this is bad code, just to look at it (though now that I do, I think this issue did crop up in several projects that I worked on).
quarks_t is a std::set of Quark (some data type, the details aren't terribly important).
data.get_servers() returns the set by value. So a temporary variable exists to hold that - but it goes out of scope in the for control statement and is not available in the loop body. So the later *it reads memory after it has been freed.
std::for_each works because the temporary (should, I think) remain in scope.
The point is that this is a subtle trap for the unwary and C++ is full of them.
Oh and you're going to get two copes of that set - one in the called function and one in the calling stack frame which is initialised with the copy constructor. Nice for efficiency, not.
Oh, on further examination I think that the begin and end iterators are coming from different instances of the same list - I need to spend some time thinking about whether that is a bug of not (I have a feeling it only works by chance). That reminds me of the fun I had with standard containers in Windows circa 1999. I had one allocated in one dll which got used in another, except that the iterators ran off the end. Turns out each dll had it's own copy of the special "end" node so code in one dll would not iterate correctly over code in another because it was checking for the address of its
own marker.
Bloody brilliant!