Reputation: 11
I am trying to write a function that removes a vector element from listAccounts depending on the account_name of that vector element. I have written this:
void Account::remove_account(string name) {
auto iter = listAccounts.begin();
for ( ; iter != listAccounts.end(); iter++) {
if ((*iter).account_name == name) {
listAccounts.erase(iter);
}
}
}
But I am getting a Segmentation fault from the vector removal, from what I understand it means that I tried to access memory that I do not have access to, but i'm not sure how to correctly write this.
Upvotes: 1
Views: 96
Reputation: 37607
If container is modified iterator becomes invalid. There are two good solutions:
void Account::remove_account(const string& name) {
auto iter = listAccounts.begin();
while iter != listAccounts.end()) {
if (iter->account_name == name) {
iter = listAccounts.erase(iter);
} else {
++iter;
}
}
}
// or
void Account::remove_account(const string& name) {
listAccounts.erase(
std::remove_if(std::begin(listAccounts), std::end(listAccounts),
[&name](const auto& item) {
return item.account_name == name;
}),
std::end(listAccounts));
}
Upvotes: 2
Reputation: 21576
Once you erase the element pointed to by an iterator, that iterator becomes invalid. (for a std::vector
, all other iterators after the erased element becomes invalid too). And incrementing or dereferencing an invalid iterator has undefined behavior.
You could do (assuming only one element is to be removed):
void Account::remove_account(string name) {
auto iter = std::find_if(listAccounts.begin(), listAccounts.end(),
[&](const auto& s){ return s.account_name == name; });
if(iter != listAccounts.end())
listAccounts.erase(iter);
}
For multiple elements, that will be:
void Account::remove_account(string name) {
for(auto iter = listAccounts.begin(); iter != listAccounts.end(); ){
iter = std::find_if(iter, listAccounts.end(),
[&](const auto& s){ return s.account_name == name; });
if(iter != listAccounts.end())
iter = listAccounts.erase(iter);
}
}
Upvotes: 2
Reputation: 310950
If you are going to remove just one element then you can write
bool Account::remove_account( std::string &name )
{
auto it = std::find_if( listAccounts.begin(),
listAccounts.end(),
[&]( const auto &item )
{
return item.account_name == name;
} );
bool success = it != listAccounts.end();
if ( success ) listAccounts.erase( it );
return success;
}
As for your code then after this statement
listAccounts.erase(iter);
the iterator becomes invalid. So you may not increment it.
Upvotes: 0