C++ std::vector entries are NULL in function, but size remains above zero

Question

It has been a while that I've written code in C/C++, and I've already found an alternative solution to my problem, but I would like to know why the original code doesn't work.

I have a test class which basically only stores a string.

class test {
private:
        std::string name;
public:
        test(std::string name) : name(name) {};
        std::string get_name() { return name; }
};

In main I have a vector which I at one point fill with test objects. The code below simulates irregular usage of the vector vect.

int main(void) {
        std::vector<test *> vect;
        std::vector<test *>::iterator i;

        //* Comment this for a working example
        std::cout << "Searching empty vector" << std::endl;
        i = *(_is_in_vector(vect, std::string("test 3")));
        if (i == vect.end()) {
                std::cout << "Nothing found" << std::endl;
        } // */

        vect.push_back(new test("test 1"));
        vect.push_back(new test("test 2"));
        vect.push_back(new test("test 3"));

        std::cout << "All:" << std::endl;
        i = *(_is_in_vector(vect, std::string("test 3")));
        if (i != vect.end()) {
                std::cout << "Erase " << (*i)->get_name() << std::endl;
                vect.erase(i);
                delete *i;
        }

        i = *(_is_in_vector(vect, std::string("test 3")));
        if (i == vect.end()) {
                std::cout << "Nothing found" << std::endl;
        }

        std::cout << "Left:" << std::endl;
        for (i = vect.begin(); i!=vect.end(); ++i) {
                std::cout << (*i)->get_name() << std::endl;
                delete *i;
        }

        vect.clear();
        return 0;
}

Because searching in the vector for a test object happens multiple times, I've created the function _is_in_vector that searches a test object and returns the iterator to it.

static std::vector<test *>::iterator * _is_in_vector(std::vector<test *> &vect, std::string find) {
        std::string identity = find;
        static std::vector<test *>::iterator i = vect.begin();
        std::cout << "Vect size: " << vect.size() << std::endl;
        for (i; i != vect.end(); ++i) {
                std::string tmp = (*i)->get_name(); /* Segmentation fault after filling vector*/
                if (0 == identity.compare(tmp)) break;
        }
        return &i;
}

The problem is that the code above works when I comment out the Searching empty vector part in main. Once the vector is filled with test objects, I call _is_in_vector a second time. The vector in this function does have three entries, but (*i) all point to NULL.

Output:

Searching empty vector
Vect size: 0
Nothing found
All:
Vect size: 3
Segmentation fault

Expected output:

Searching empty vector
Vect size: 0
Nothing found
All:
Vect size: 3
Erase test 3
Vect size: 2
Nothing found
Left:
test 1
test 2

Function name `_is_in_vector` is illegal in C++. This is most probably unrelated to your problem but you should be aware. — Slava, Jun 06 '19 at 09:49
`_is_in_vector()` returns a pointer to a local variable, which ceases to exist when the function returns. That causes the caller to receive a dangling reference, and subsequent usage of that reference gives undefined behaviour. The name of the function is also reserved in the global namespace, so also (in itself) causes undefined behaviour. — Peter, Jun 06 '19 at 09:53
Oh, in that case it's just appalling style, but not UB. Dereferencing `i` after passing it to `erase` however, _is_ still UB. — Useless, Jun 06 '19 at 09:56
`vect.erase(i);` and then `delete *i;` invalidates the iterator i. You might want to delete first and then erase. — Samer Tufail, Jun 06 '19 at 09:56
NB. if you change the declaration of `static ... i` so you set it on every call instead of only initializing once - it'll still be appalling but won't actually be broken there. Obviously problem with dereferencing an invalidated iterator also needs fixing. — Useless, Jun 06 '19 at 09:59

Slava · Accepted Answer · 2019-06-06T11:19:07.647

First of all it is unclear why you need to store test objects by pointer but not by value. If you do need it use smart pointer.

As for your problem, why do you return pointer to an iterator? This is the root cause of your problem - to make &i legal to return you made it static, but static local variables initialized only once and do not change value btw calls - so after first call it pointed to an element in the vector, but after that you added elements and invalidated all iterators including static i hense the segmentation fault. So fix is simple - return iterator by value and make i non static but regular, it is light and it is totally fine to do so.

PS Identifiers starting with _ are illegal in global context, details can be found here What are the rules about using an underscore in a C++ identifier?

So your function actually should look like this:

static std::vector<test *>::iterator  is_in_vector( std::vector<test *> &vect, const std::string &find) 
{
      return std::find_if( vect.begin(), vect.end(), [find]( test *p ) {
          return p->get_name() == find;
      } );
}

assuming the vector should never hold nullptr, if it is the case or to play safe change condition to:

          return p && p->get_name() == find;

With your input I'd solved it already, but not with `std::find_if`. I will check it out. Thanks for the extra effort :-) — Bayou, Jun 06 '19 at 10:40

C++ std::vector entries are NULL in function, but size remains above zero

1 Answers1