How do I use std:vector's erase() function properly?

Question

I am having a strange issue when using the erase() function on a std:vector. I use the following code:

int count = 0;
for (int itr=0; itr<b.size(); ++itr) {
    if (b[count].notEmpty = false) {
        b.erase(b.begin()+count);
        --count;
    }
    ++count;
}

However, for some reason there are no elements actually getting erased from b. b is declared elsewhere as follows:

vector<block_data> b;

Where block_data is a structure, and contains the boolean value notEmpty. Some of b's elements are properly being assigned with notEmpty = false earlier in the code, so I am not sure why they aren't getting erased. Is it an error with the syntax, or something else?

For the record, the double-negative logic of testing `!x.notEmpty` (x is not non-empty) is a sign that you should rename `notEmpty` to `empty`. — Jon Purdy, Jul 21 '11 at 23:11

score 8 · Accepted Answer · answered Jul 21 '11 at 22:43

8

There's nothing wrong with your use of erase. The problem is an assignment inside the if condition:

if(b[count].notEmpty = false)

This sets b[count].notEmpty to false, then returns false. This will cause the inner-body of the if-statement to never run.

Change it to

if(b[count].notEmpty == false)

or event

if(!b[count].notEmpty)

And you should be good to go.

answered Jul 21 '11 at 22:43

Peter Alexander

53,344
14
119
168

+1 LOL I read the OP's code and totally missed the assignment. – salezica Jul 21 '11 at 22:56
2

@Mike N.: Don't worry. This is an incredibly common problem. Try turning compiler warnings on though, as most compilers do provide warnings for this particular case. – Peter Alexander Jul 21 '11 at 23:03

Luc Danton · Answer 2 · 2011-07-21T23:35:21.433

3

Others have pointed out how to fix your code, but just in case: how to use a Standard algorithm.

// Decide if an element is to be removed
auto predicate = [](block_data& b)
{
    // more idiomatic than b.notEmpty == false
    return !b.notEmpty;
});

// Remove
auto removed = std::remove_if(b.begin(), b.end(), predicate);

// Count
auto count = b.end() - removed;

// Erase.
b.erase(removed, b.end());

edited Jul 21 '11 at 23:35

answered Jul 21 '11 at 23:08

Luc Danton

34,649
6
70
114

People should have their keyboards taken away from them for writing such code. You can use `std::remove_if` of course, but why do you have to make your code so ugly and unreadable? – littleadv Jul 21 '11 at 23:16
1

@littleadv Don't read it all at once. Standard algorithms are building blocks, and here I use them to separate the responsibilities. If you put the predicate on a separate line, here are the 4 things that happen: the bit of code that decides whether an element gets the axe or not, the bit of code that isolate unwanted elements, the bit of code that counts how many elements were isolated, and the bit of code that erases the isolated elements. If you're convinced one individual is correct in isolation, then you quickly know the algorithm as a whole also is. – Luc Danton Jul 21 '11 at 23:30
2

@littleadv : This is far more readable than nested loops and condition statements. Also, I would say your suggestion of `false == b[count].notEmpty` is the ugliest piece of code in this thread. ;-] – ildjarn Jul 21 '11 at 23:33
@Luc Danton - now that you edited it and don't pass a function body as an argument - it is OK. You don't have to take it personally, especially if you agree with me. – littleadv Jul 22 '11 at 00:21
1

@littleadv Does that mean I get my keyboard back :) ? – Luc Danton Jul 22 '11 at 00:26
@Luc, your keyboard privileges are officially restored. :p – littleadv Jul 22 '11 at 00:33

score 2 · Answer 3 · answered Jul 21 '11 at 22:41

2

b[count].notEmpty = false should be b[count].notEmpty == false, otherwise the if will always be false.

Better practice to write false == b[count].notEmpty, in this way the constant on the left is not an l-value, and if you make the (quite common) mistake of writing = instead of == you'll get a compilation error.

answered Jul 21 '11 at 22:41

littleadv

20,100
2
36
50

Putting the constant of the left works, but most compilers I've worked with emit a warning if you perform an assignment within a conditional statement. – Praetorian Jul 21 '11 at 22:48
@Praetorian - they might, but then warnings don't stop compilation and are so easy to ignore... I wouldn't (and don't) rely on that. – littleadv Jul 21 '11 at 22:50
2

They do stop compilation if you set the compiler to treat warnings as errors. You should do this! – Peter Alexander Jul 21 '11 at 22:53
@Peter - I certainly do, but not everyone is in control of their build environment. – littleadv Jul 21 '11 at 23:14
1

If you "commonly" make that error, then it will go unnoticed half the time (because you're far from always comparing against a constant). The only robust option is to learn to use `==` correctly. And then you can put the `false` on the right hand side where it makes intuitive sense and it's not so goddamn ugly. ;) – jalf Jul 22 '11 at 06:34
@jalf - I'm sure you're being sarcastic, you don't really think I don't know the difference between `=` and `==`, right? If you're not being sarcastic - there's a new word for you to learn: **typo**. Noone makes such an error intentionally, including the OP, I'm sure. – littleadv Jul 22 '11 at 08:29
I'm not implying that you don't know the difference. I'm saying that it is possible, with a bit of practice, to stop *making* that error. And that is really the only way to reliably guard against it. In particular, relying on the "constant on the left side" trick is iffy because it only protects you from the easy-to-detect cases. Beginners often type a = instead of ==, but with experience, the difference becomes so intuitive that you type the right one without thinking. And then you don't need to rely on the "constant on the right" rule. – jalf Jul 22 '11 at 10:26
@jalf - If only everyone was so lucky to not ever make typos as you.... Whatever, you don't like constants on the left - so be it. – littleadv Jul 22 '11 at 17:41

How do I use std:vector's erase() function properly?

3 Answers3