Finding NULL pointers in std vectors

Question

I'm working with vectors and at some point there will be NULL entries; I want to erase all NULL occurrences within the given vectors. My approach so far is not working:

for(int i = sent_flit_list->size() - 1; i >= 0; i--)
if(sent_flit_list[i] == NULL)
    sent_flit_list->erase(sent_flit_list[i]);

for(int i = sent_pkt_list->size() - 1; i >= 0; i--)
if(sent_pkt_list[i] == NULL)
    sent_pkt_list->erase(sent_pkt_list[i]);

Where

vector<Flit*> *sent_flit_list;
vector<Packet*> *sent_pkt_list;

are the vectors. I have tried casting to a type (Flit*)NULL/(Flit*)0 but with no success.

Any help will be greatly appreciated.

Secretin

What exactly do you mean by "is not working"? What happens? How does it differ from what you wanted to happen? – Sheeree

IIRC, vector::erase takes an iterator, not a value, as a parameter. – Cle

@Gareth Gives an error such as error C2678: binary '==' : no operator found... I simply want to remove anything that points to NULL in those vectors. – Secretin

You should really be using remove for this rather than looping explicitly. I think the immediate cause of your problem is that sent_flit_list and sent_pkt_list are pointers to vectors, not vectors, so that when you say e.g. sent_pkt_list[i] that indexing operation isn't what you think it is. (This is not the only problem with the code, but it's the one that's causing that error message.) – Sheeree

Another Hint which nobody mentioned so far, remove if this is wrong: We should never erase elements and looping over the vector with out iterators. Because Suppose you have 2 values in std::vector a you iterate over the vector with indexing like you do, now erase value at position one, the 2 index is now wrong because the vector shrinked... Always use iterators (it) and use it=a.erase(it) – Damales

Answer 1

Use the Erase-Remove idiom to remove elements based on a predicate from a container.

In your case:

// with a predicate
my_vec.erase(std::remove_if(begin(my_vec), end(my_vec), 
                           [](Flit* x) { return x == nullptr; }), 
             end(my_vec));

// with a value value
my_vec.erase(std::remove(begin(my_vec), end(my_vec), nullptr),
             end(my_vec));

Your current approach isn't working, because vector::erase expects an iterator to an element of the vector and not a value of the stored type.

Frankly, what you are doing seems a little bit strange. You shouldn't store pointers, but values in containers. If you require nullable values, use a Maybe class such as boost::optional.

Megargee

What's wrong with a vector of pointers? Or even smart pointers? – Mitziemitzl

@Johan: Something like boost::optional will assert that it's initialized, unlike possible null-pointer problems. However, if you have large objects, such a Maybe class is not always an option. – Satisfied

@Johan: Nothing, in fact. Sometimes, it is the right thing to do. However, the OP is using a bit too many of them to be considered normal (a pointer to a vector of pointers, twice). Granted, we don't know the exact situation, but the fact that the OP is using that much pointerage, combined with the fact that he is beginner enough to not even know about std::remove, indicates he is a little over-zealous about them, and probably doesn't need them at all. – Ornament

But using end() iterator in erase() is illegal: all iterators supplied to erase() must be valid AND derefernceable. – Focus

Answer 2

pmr is absolutely correct that you should be using remove followed by erase, and that this is the most important mistake in the code. However, the mistake that's actually causing the error message you report is as follows:

Your variables sent_pkt_list and sent_flit_list are pointers to vectors, not vectors. Therefore, when you say something like sent_pkt_list[i], this is doing C-style array indexing, not vector indexing. The value of sent_pkt_list[i] is a (doubtless nonsensical because it's effectively dereferencing a bogus pointer) vector<Packet*>, not a Packet*. So you then try to compare that against NULL, which of course doesn't work.

Sheeree

Personally, I think pmr's answer should have the upvotes (because, really, it's what the questioner needs to know) and mine should be accepted (because it happens to explain the specific error the questioner was troubled by). But life's too short to worry about whether any given SO answer has been treated exactly optimally by the voting-and-accepting public! – Sheeree

Answer 3

// Full example for people who can have something equal to explained task and avoid read tones of discussions.

#include <iostream>
#include <vector>
#include <algorithm>

struct Flit {};
struct Packet {};

int main()
{
    std::vector<Flit*> sent_flit_list_(10);
    std::vector<Packet*> sent_pkt_list_(20);

    std::vector<Flit*> *sent_flit_list = &sent_flit_list_;
    std::vector<Packet*> *sent_pkt_list = &sent_pkt_list_;

    sent_flit_list->erase(std::remove(sent_flit_list->begin(),sent_flit_list->end(), nullptr), sent_flit_list->end());
    sent_pkt_list->erase(std::remove(sent_pkt_list->begin(),sent_pkt_list->end(), nullptr), sent_pkt_list->end());
    return 0;
}

Lex