3

I need to create several objects and put them in a list (for which I am using std::vector). Also, I need the list items to point to the addresses of the objects so that the changes I make to the objects are reflected in the list too. But the thing is, every item in the list is pointing to the last object created in the loop. 

    for(int i=0;i<50;i++){
        for(int j=0;j<50;j++){
            Grass g1;
            g1.position.x = i;
            g1.position.y = j;
            grassList.push_back(&g1);
        }
    }

The the attributes of grass objects in the list should be..

[0,0]
[0,1]
[0,2]
.
.
.
.
[49,49]

But it's coming out to be..

[49,49]
[49,49]
[49,49]
[49,49]
.
.
.
[49,49]
Ankur Lathwal
  • 587
  • 1
  • 5
  • 20
  • 2
    Either push by value, or create a new Grass object (as in using the "new" operator). g1 is created on the stack, and is basically the same object whose address you are using over and over again. – OldProgrammer Apr 20 '16 at 00:59

3 Answers3

3

You're pushing pointers to local variables to the vector. Local variables get destroyed at the end of their scope (the second-to-last } in this case). Therefore, dereferencing any of those pointers after the } is undefined behavior. The output you're seeing is a completely valid result of undefined behavior.

Using pointers here just doesn't make sense. Use them (including new) only when absolutely necessary. For more info, see Why should I use a pointer rather than the object itself?

This is the right way:

std::vector<Grass> grassList;
         // ^^^^^ not pointers!

for(int i=0; i<50; ++i) {
    for(int j=0; j<50; ++j) {
        Grass g1;
        g1.position.x = i;
        g1.position.y = j;
        grassList.push_back(g1);
    }
}
Community
  • 1
  • 1
emlai
  • 37,861
  • 9
  • 87
  • 140
  • "*Using pointers here just doesn't make sense*" may be too absolute. It is likely that in most cases using a vector of objects works well, but the specific needs of the OP *might* as well not be covered. The accepted answer in the question linked is somewhat more cautious: "*You should only use dynamic storage duration when you need it.*" – sancho.s ReinstateMonicaCellio Apr 20 '16 at 01:26
  • I wasn't using pointers in the beginning. I was doing it exactly the same way you wrote above. But the problem is, when I was making changes to those objects in the rest of the code (outside of the vector list), they weren't reflecting in the list (for obvious reasons). Perhaps I should directly reference the objects from the list itself. Thanks for making it clear. – Ankur Lathwal Apr 20 '16 at 03:48
2

If you're accustomed to other languages that use ref-counting of variables, you might have expected that

Grass g1;

was creating a new "Grass" object every iteration of the loop. It's not, C++ isn't a ref-counted language.

Instead, it creates a scoped local variable on the stack.

Because you're doing this in a loop, it's probably going to be at the same location in memory every time.

You will either need to:

  1. Instead of pointers, just make your container a container of Grass objects: let the container handle allocation for you.

  2. Use C++11's unique_ptr and C++14's make_unique to create an instance of Grass dynamically for each iteration of the loop. When the vector containing the unique_ptrs goes out of scope, they will be automatically freed.

  3. Use the new and delete keywords to manually allocate and release Grass objects to point to.

Option 1:

#include <vector>

struct Grass {
    struct {
        int x, y;
    } position;
};

int main() {
    std::vector<Grass> grassList;
    for(int i=0;i<50;i++){
        for(int j=0;j<50;j++){
            Grass g1;
            g1.position.x = i;
            g1.position.y = j;
            grassList.push_back(g1);
        }
    }  
}

Live demo: http://ideone.com/DQs3VA

Option 2:

#include <memory>
#include <vector>

struct Grass {
    struct {
        int x, y;
    } position;
};

int main() {
    std::vector<std::unique_ptr<Grass>> grassList;
    for(int i=0;i<50;i++){
        for(int j=0;j<50;j++){
            auto g1 = std::make_unique<Grass>();
            g1->position.x = i;
            g1->position.y = j;
            grassList.push_back(std::move(g1));
        }
    }   
}

Live demo: http://ideone.com/hJUdwR

Option 3:

#include <vector>

struct Grass {
    struct {
        int x, y;
    } position;
};

int main() {
    std::vector<Grass*> grassList;
    for(int i=0;i<50;i++){
        for(int j=0;j<50;j++){
            Grass* g1 = new Grass;
            g1->position.x = i;
            g1->position.y = j;
            grassList.push_back(g1);
        }
    }
    // ...
    for (auto& g1: grassList) {
        delete g1;
    }
    grassList.clear();
}

Live demo: http://ideone.com/GTk7ON

kfsone
  • 21,566
  • 2
  • 35
  • 66
0

If for whatever (unspecified) reason you need to store the pointers to the locations of the objects, you need this

std::vector<Grass*> grassList;  // Assumed you already have something like this
for(int i=0;i<50;i++){
    for(int j=0;j<50;j++){
        Grass* gp1 = new Grass;
        gp1->position.x = i;
        gp1->position.y = j;
        grassList.push_back(gp1);
    }
}

This avoids the destruction of the objects pointed at by the pointers you are storing. See, e.g., Creating an object: with or without `new` You will have to take care of suitably deleting objects with delete to free memory.

But depending on your needs, you may also use an array of objects instead of an array of pointers, as suggested by tuple_cat.

Community
  • 1
  • 1
sancho.s ReinstateMonicaCellio
  • 12,097
  • 14
  • 65
  • 141