Uwe Stöhr wrote:
I'm still a bloody C++ beginner and lost when looking into your code.
No need to be lost. Let's see if I can explain.
The best thing would be to provide a patch tho make the void
"handle_opt" work without the need to have a "delete_void" called
afterwards.
It's best to keep the two separate. Removing elements from a vector
within a loop invalidates the loop iterators.
Anyway, the explanation...
I was commenting on your delete_opt function, because its inefficient
and because the STL provides tools to achieve your aim in a less
expensive manner.
What's inefficient about your existing code?
+ for (; *what; ++what) {
+ it = find(opts.begin(), opts.end(), *what);
+ if (it != opts.end())
+ opts.erase(it);
+ }
This loops over all strings in the what array of strings. It looks for
each string "*what" in the "opts" vector<string> and, if it finds it,
it removes it. That's expensive, because you're regenerating the
"opts" vector completely with each call to opts.erase.
The STL provides a means to achieve the same using std::remove and
std::erase.
std::remove will move all iterators matching your predicate to the end
of the collection. It's possible to do that multiple times, resetting
the "new_end" iterator to mark (1 past) the end of that part of the
collection you're interested in. Then use std::erase to actually
resize the collection only once.
std::remove comes in two flavours, one that takes a variable that can
be compared directly to an element of opts, and another, called
std::remove_if, that takes a functor as its predicate. What's a
functor? Just a fancy name to say a class variable that overrides
operator(). It's a very powerful STL concept.
Let's look at your example again, but let's use a vector<string> to
hold the things we want to discard. Just to make the code easier to
understand.
void delete_opt(
vector<string> & opts,
vector<string> const & to_discard)
{
if (opts.empty() || to_discard.empty())
return;
// Use a functor, element_matches, to decide which elements
// of opts to move to the end. This functor has a constructor
// that takes the collection of strings we want to
// remove from opts.
vector<string>::iterator new_end = std::remove_if(
opts.begin(), opts.end(), element_matches(to_discard));
// Only one resize is needed...
std::erase(new_end, opts.end());
}
All that remains is the definition of the functor. As I said above,
it's a class with a constructor that takes the collection of strings
we want to remove:
struct element_matches {
private const vector<string> const & data_;
element_matches(vector<string> const & data) : data_(data) {}
};
However, to make it a *functor* that can be used by std::remove_if, we
need to give it an operator() overload that takes an element of "opts"
as its single parameter and returns a bool meaning "remove", so:
struct element_matches {
private vector<string> const & data_;
element_matches(vector<string> const & data) : data_(data) {}
bool operator()(string const & opt_element) const
{
vector<string>::const_iterator it = data_.begin();
vector<string>::const_iterator const data_end = data.end();
for (; it != data_end; ++it)
{
if (opt_element == *it)
return true;
}
return false;
}
};
So, I end up with the same number of comparisons as you do, but with
only one resize of the opts vector.
Makes sense now?
Regards,
Angus
ps. As one final piece of trickery, you'll notice that the body of
operator() above is no more than the body of std::find. So, it could
be written:
bool operator()(string const & opt_element) const
{
vector<string> iterator it = std::find(
data_.begin(), data.end(), opt_element);
return it != data.end();
}
Hope this helps ;-)
A.
