Where you use while() I would prefer to use for(;;) (and use pre-increment).
Not a big deal but I like to keep all the loop control in one place
auto it = arr.begin();
while (it != arr.end()) {
// STUFF
it++;
}
// I prefer (though not much different)
for(auto it = arr.begin(); it != arr.end(); ++it)
{
// STUFF
}
Rather than have SieveHelper being declared once and then updated each time through the loop. Just create a new one each iteration. The actual cost of construction will be optimized to zero and it becomes easier to read.
helper.cur = *it;
auto last = std::remove_if(it, arr.end(), helper);
// I would do this (note it requires slight modifications to SieveHelper (see below)).
auto last = std::remove_if(it, arr.end(), SieveHelper(*it));
Keeping const correct is a good habit to get into. You need to apply this to your SieveHelper. Also for simple helper functions like this I like to make them structs
struct SieveHelper
{
bool operator()(unsigned int i) const // object not changed by method => const
// ^^^^^
{
return i != cur && i % cur == 0;
}
// Use a reference (as we don't need to store state in the helper)
SieveHelper(unsigned int const& c) : cur(c) {}
private:
unsigned int const& cur;
};
Since you are using C++11 we can even simplify this to a lambda
auto last = std::remove_if(it, arr.end(),
[&it](unsigned int i){ return i != (*it) && i % (*it) == 0;}
);
As an optimization (probably not a big one) you don't need to call erase after each iteration:
void sieve(std::vector<unsigned int>& arr)
{
SieveHelper helper;
auto it = arr.begin();
auto last = arr.end()
while (it != arr.end()) {
helper.cur = *it;
last = std::remove_if(it, last, helper);
it++;
}
arr.erase(last, arr.end());
}
Now if we apply all these (apart from lambda)
void sieve(std::vector<unsigned int>& arr)
{
auto last = arr.end()
for(auto it = arr.begin(); it != arr.end(); ++it)
{
last = std::remove_if(it, last, SieveHelper(*it));
}
arr.erase(last, arr.end());
}
