for each loop read access violation

Question

I've been writing an engine and ran into a problem, I'm getting a read access violation in my code. I have no idea why, but it happens when I add this->chunks.push_back(chunk); into this block of code,

void c_world::generate_world()
{   
    this->chunks.resize(MAX_CHUNKS);

    for (auto chunk : this->chunks)
    {
        chunk.setup_landscape();
    }
}

so it becomes...

void c_world::generate_world()
{   
    this->chunks.resize(MAX_CHUNKS);

    for (auto chunk : this->chunks)
    {
        chunk.setup_landscape();
        this->chunks.push_back(chunk);
    }
}

this->chunks is an std::vector<c_chunk> (c_chunk) being a custom class. Thank you!

All helps appreciated.

EDIT: this was the right way to do it..

void c_world::generate_world()
{   
    for (std::uint32_t i = 0; i < MAX_CHUNKS; i++)
    {
        c_chunk chunk[MAX_CHUNKS];
        chunk[i].setup_landscape();
        this->chunks.push_back(chunk[i]);
    }
}

I don't know what type of object `chunks` holds, but `for (auto chunk : this->chunks)` is making copies of them and `chunk.setup_landscape();` is applied to those copies. It *might* be what you want, but I doubt it. — François Andrieux, Mar 02 '18 at 21:50
Even if the second version did work without invalidating the iterators, it wouldn't make much sense. You would end up with a vector containing `MAX_CHUNKS` uninitialized chunks followed by `MAX_CHUNKS` initialized chunks. I doubt that's what you were going for. — Miles Budnek, Mar 02 '18 at 21:55
@MilesBudnek exactly, thats why I'm trying to push it back. So my problem is, how do I push it back correctly? (Without this crash?) — Graham Best, Mar 02 '18 at 21:56
`vector::push_back` appends to the end of the vector. Using `push_back` like that you will end up with `chunks.size() == MAX_CHUNKS * 2`. If you want to modify each chunk in place, you need to change the loop variable to a reference. — Miles Budnek, Mar 02 '18 at 21:58
@GrahamBest You can safely push_back into a vector you are currently iterating over as long as you do not cause the vector's size to exceed it's capacity. — François Andrieux, Mar 02 '18 at 21:58
@MilesBudnek You cannot assume how many elements the vector will contain with the way OP is doing it, since it's undefined bejavior. Though, it does seem like what OP intends to do would lead to a vector containing `MAX_CHUNKS * 2` vectors. — François Andrieux, Mar 02 '18 at 21:59
@FrançoisAndrieux True, my comments are all assuming you have reserved enough space before you begin iteration that the iterators won't be invalidated as you append elements. — Miles Budnek, Mar 02 '18 at 22:01
@FrançoisAndrieux Well, you see, the problem is that the very first iteration causes this error, I highly doubt its exceeding its max capacity. — Graham Best, Mar 02 '18 at 22:01
@GrahamBest The first `push_back` after an explicit `resize` will almost certainly cause the vector to allocate new storage. — Miles Budnek, Mar 02 '18 at 22:02
@GrahamBest Why would you assume that? You only seem to call `resize`, so it's quite possible the capacity is already exactly the same as the size. Any `push_back` would then invalidate iterators. See [`std::reserve`](http://en.cppreference.com/w/cpp/container/vector/reserve). You should reserve enough capacity for your vector to accommodate all of the elements you intend to push, in addition to all the elements it already contains. — François Andrieux, Mar 02 '18 at 22:02
@FrançoisAndrieux It contains _no_ elements though, so my problem is how do I add some? While going over MAX_CHUNKS correctly? — Graham Best, Mar 02 '18 at 22:04
@GrahamBest `chunks` contains `MAX_CHUNKS` (default-constructed) elements because you just resized it. If it didn't contain any elements, your loop would never be entered in the first place. — Miles Budnek, Mar 02 '18 at 22:06
@MilesBudnek I thought resizing doesn't add any elements? Check my edit — Graham Best, Mar 02 '18 at 22:07
@GrahamBest No, `reserve` doesn't add elements. `resize` does. — Miles Budnek, Mar 02 '18 at 22:09
@GrahamBest See [this answer](https://stackoverflow.com/a/7397862/7359094). I believe you may misunderstand what capacity and size are. Capacity is how many elements the vector could store without having to get more memory. The size of the number of actual elements it currently contains. Resize will create (or destroy) as many elements as is required for the vector to contain the requested number. A range-based for loop will iterate over the elements the vector currently contains (it's size). If it was empty, the loop would not run at all. — François Andrieux, Mar 02 '18 at 22:10
@GrahamBest -- Your last edit looks strange. You are declaring a local array `chunk` inside that loop. You are not iterating over that array, instead you're creating a new local array on each iteration. There is no need for an array, just declare a single `chunk`, set it up, and then `push_back` that single `chunk` into the vector. — PaulMcKenzie, Mar 02 '18 at 22:29
@GrahamBest Yes, and pushing a car instead of just starting the engine and driving it also "works fine" if you want to get somewhere. The point is that you're using an array for absolutely no reason in that loop. — PaulMcKenzie, Mar 02 '18 at 23:00
@PaulMcKenzie It works fine, and doing what you just said doesn't work, — Graham Best, Mar 02 '18 at 23:16
@GrahamBest Then either you didn't [do something like this](http://coliru.stacked-crooked.com/a/c6f4376aa90c17e6), or your program has corrupted memory before that loop is encountered. The code at that link has to work correctly, or else your program has some serious undefined behavior / memory corruption going on before that loop executes. — PaulMcKenzie, Mar 02 '18 at 23:20
@PaulMcKenzie I assume you didn't fully understand what I was trying to do. — Graham Best, Mar 02 '18 at 23:23
@GrahamBest I am looking at what you say is the "right way to do this". What is different in what I posted and what you posted? You needlessly declare a *local* array inside the loop (that gets destroyed on each iteration), and fill one spot in that array, and push_back that one spot. So instead of an array, just declare one chunk, and push_back that one chunk. Unless you can explain how that is different, I am all ears. — PaulMcKenzie, Mar 02 '18 at 23:25

score 1 · Answer 1 · answered Mar 02 '18 at 22:09

1

Others have said why your program crash. Here is depending on what you want exactly to do two possible solutions:

If you want to have MAX_CHUNKS uninitialized then MAX_CHUNKS initialized, go with a simple for loop.

void c_world::generate_world()
{   
    this->chunks.resize(MAX_CHUNKS);
    for (int i = 0;i < MAX_CHUNKS;i++)
    {
        this->chunks.push_back(this->chunks[i])
        this->chunks[i+MAX_CHUNKS].setup_landscape();
    }
}

If you want to have MAX_CHUNKS initialized then:

void c_world::generate_world()
{   
    this->chunks.resize(MAX_CHUNKS);
    for(auto &chunk : this->chunks)
        chunk.setup_landscape();
}

answered Mar 02 '18 at 22:09

MaikuZ

56
3

Your first example is wrong however, you're going out of bounds of the vector, correct? – Graham Best Mar 02 '18 at 22:11
@FrançoisAndrieux No, there is no out of size. You push_back first an element, so the size is bigger by 1. – MaikuZ Mar 02 '18 at 22:12
When would it crash then? – MaikuZ Mar 02 '18 at 22:14
@MaikuZ My mistake, I thought it was a range-based for loop, but I now see it uses indices. Though I will recommend using `chunks.back()` to access the last element for clarity. – François Andrieux Mar 02 '18 at 22:15
Check my edit. :-) – Graham Best Mar 02 '18 at 22:24

score 0 · Answer 2 · answered Mar 02 '18 at 22:15

0

The problem is that you add elements to your chunks vector inside for loop. So vector changes inside the loop and iterators used by the loop become invalidated.

answered Mar 02 '18 at 22:15

dividedbyzero

136
1
5

Check my edit. :-) – Graham Best Mar 02 '18 at 22:24

for each loop read access violation

2 Answers2