Reputation: 725
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]
Upvotes: 4
Views: 1779
Reputation: 24269
(Modern-C++ update at the end)
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:
Instead of pointers, just make your container a container of Grass objects: let the container handle allocation for you.
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.
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
C++11 introduced emplace_back
which lets you allocate and construct the entry in the container in one go.
#include <vector>
struct Grass {
struct {
int x, y;
} position;
// because x and y are inside a private sub-struct,
// we'll need a constructor to pass the values in.
Grass(int x, int y) : position{x, y} {}
};
int main() {
std::vector<Grass> grassList{}; // default initialized.
static constexpr size_t dim = 10; // name the constant (DIMension)
grassList.reserve(dim); // allocate memory in advance
for (auto i = 0; i < dim; i++) {
for(auto j = 0; j < dim; j++) {
grassList.emplace_back(i, j);
}
}
// no cleanup required
}
live demo: https://gcc.godbolt.org/z/G1YsW7hMs
Upvotes: 5
Reputation: 15641
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.
Upvotes: 0
Reputation: 42888
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);
}
}
Upvotes: 4