Why are vector contents changed after the method scope finishes?

Question

The following dummy program mimics the behavior of another program I'm troubleshooting.

#include <iostream>
#include <vector>

class A
{
public:
    std::vector<int> data;

    void DoTheThing()
    {
        while (data.size() < 10) {
            data.push_back(1);
        }
    }
};

class B
{
public:
    std::vector<A> objs;

    B()
    {
        A one, two, three;
        objs.push_back(one);
        objs.push_back(two);
        objs.push_back(three);
    }

    void DoTheThing()
    {
        for (auto obj: objs) {
            obj.DoTheThing();
            std::cout << "DEBUG length during=" << obj.data.size() << std::endl;
        }
    }
};

int main()
{
    B b;
    b.DoTheThing();
    for (auto obj : b.objs) {
        std::cout << "DEBUG length  after=" << obj.data.size() << std::endl;
    }
}

I compile and run as:

$ g++ -Wall --std=c++11 -o test test.cpp
$ ./test
DEBUG length during=10
DEBUG length during=10
DEBUG length during=10
DEBUG length  after=0
DEBUG length  after=0
DEBUG length  after=0
$

For some reason the state of the A objects in b's objs vector is changing between the b.DoTheThing() call and the subsequent print statements. My question is what is happening? Are the A object data vectors going out of scope somehow and being deleted, or perhaps the entire A objects? It seems like a scoping issue--perhaps even a trivially simple one--but it's been long enough since I programmed in C++ that I'm not sure. How can I make the contents of the data vectors persist after the call to b.DoTheThing() in the other methods?

score 6 · Accepted Answer · edited May 23 '17 at 10:28

"For some reason the state of the A objects in b's objs vector is changing between the b.DoTheThing() call and the subsequent print statements. My question is what is happening?"

void DoTheThing()
{
  for(auto obj : objs)
   // ^^^^^^^ receive by value due to implicit copy

You are actually making a separate copy of objs into individual temporary objs. Those temporaries are destroyed after the B::DoThething() finishes. To avoid the copies, use reference:

  for(auto& obj : objs)
   // ^^^^^^^ receive by reference

The same is true for the similar loop in the main() as well.

Real question can be: "How to avoid such accidents?"

If you can afford, then make the copy constructor as explicit to avoid implicit copying:

class A {
public:
  A () = default;      
  explicit A(const A&) = default;  // implicit copy is avoided
  A (A&&) = default;  // to avoid RVO related issues
  ...
};

Here is a demo, which shows how the explicit generates compilation error to catch the problem in the for loop which accidentally makes copies.

The explicit brings its own syntax limitations. Some of them can be solved & some cannot. Refer following question for 1 such issue (& how it's solved):

What is the effect of 'explicit' keyword on the Return Value Optimization (RVO)?

Making a copy constructor `explicit` is likely to cause other problems. It's better to just learn the language to understand where copies are made, and avoid it them if needed. — Jonathan Wakely, Aug 16 '16 at 07:05
@JonathanWakely, 1 of such known issues is the RVO thing. That can be solved in certain situations, which I have updated in the answer. Yes, `explicit` comes with its own limitation, but I find them worthy. I had been into exactly this situation few weeks back & it was really difficult to debug. We can also have a `#ifdef/#endif`s & create a `#define conditional_explicit explicit` under it. This can be useful to eventual check of any such copies & revert back to normal. — iammilind, Aug 16 '16 at 07:12

score 2 · Answer 2 · answered Aug 16 '16 at 06:38

The problem is in the range for loop:

void DoTheThing()
    {
        for (auto obj: objs) {
            obj.DoTheThing();
            std::cout << "DEBUG length during=" << obj.data.size() << std::endl;
        }
    }

You are copying the A object and doing the modification in the copy, not in the original one.

Change it to:

for (auto& obj: objs) {
  ...
}

and it should work as expected.

score 2 · Answer 3 · answered Aug 16 '16 at 06:38

2

You have to change your for loops as follows:

for (auto& obj : b.objs)

In this way you handle the actual object in the container by reference. If you declare it as:

for (auto obj : b.objs)

obj will be a copy

answered Aug 16 '16 at 06:38

dau_sama

4,247
2
23
30

Awesome. I knew it was simple. Thanks! – Daniel Standage Aug 16 '16 at 06:40
you can have fun with `for_each` as well as next step :-) – dau_sama Aug 16 '16 at 06:43

Shravan40 · Answer 4 · 2016-08-16T07:07:56.860

0

Instead of receive by value you should use receive by reference.

for (auto obj : b.objs) {
        std::cout << "DEBUG length  after=" << obj.data.size() << std::endl;
    }

Above piece of code is receive by value, Change it to

for (auto& obj : b.objs) {
        ........
    }

edited Aug 16 '16 at 07:07

answered Aug 16 '16 at 06:47

Shravan40

8,922
6
28
48

Why are vector contents changed after the method scope finishes?

4 Answers4