CODEWITHSUNDEEP
c++stl
Favolas
Favolas

Reputation: 7243

C++ error erasing element of vector of deque

I have the following code:

deque<int> assigned_g_col_ids;
vector < deque<int> > assigned_g(MAX_P);
deque<int>::iterator it;

int distribute_gifts(int collegue) {
int position = 0;
for (int i = 0; i < number_of_presents; i++) {
    if (present_preferences[collegue][i] && available_gifts[i] > 0) {
        assigned_gifts[i].push_back(collegue);
        available_gifts[i]--;
        return 1;
    } else if (present_preferences[collegue][i] && !(available_gifts[i] > 0) && visited_gifts[i] == 0) {        
        visited_gifts[i] = 1; 
        assigned_gift_collegues_ids = assigned_gifts[i];
        for (it = assigned_gift_collegues_ids.begin(); it != assigned_gift_collegues_ids.end(); it++) {
            if (distribute_gifts(*it)) {
                assigned_gifts[i].erase(assigned_gift_collegues_ids.begin());
                assigned_gifts[i].push_back(collegue);

                return 1;
            }
            position+=1;
        }
    }
}
return 0;

}

I'm getting memory error at assigned_g[i].erase(assigned_g_col_ids.begin() + position);

How can I erase the element pointed by it from the deque if the return value of distribute_g(*it) is 1?

Upvotes: 0

Views: 149

Answers (2)

David Hammen
David Hammen

Reputation: 33116

There are a number of things wrong here. A minor problem: It's best to adopt the habit of using prefix rather than suffix autoincrement and autodecrement (e.g., ++it rather than it++).

A much bigger problem is that you have declared deque<int>::iterator it as a file scope variable. You are using that variable in multiple contexts because your function distribute_g is recursive. Those recursive calls will do who knows what to that global iterator. The iterator should be local to the for loop in which it is used.

A related problem is that the iterator is invalidated by calling erase. Your code is immediately returns after calling erase, so it would appear that this is safe. However, you are calling distribute_g recursively. You need to reset the iterator after that recursive call to distribute.

Finally, what is the rationale for the recursive calls?

Update
Your updated code is illegal (and so was your original code).

Here's the crux of your problem:

assigned_gift_collegues_ids = assigned_gifts[i];
...
assigned_gifts[i].erase(assigned_gift_collegues_ids.begin());

That assignment to assigned_gift_collegues_ids makes a copy of assigned_gifts[i]. The iterator returned by assigned_gift_collegues_ids.begin() points to the start of the contents of that copy. It is illegal to use this iterator as an argument to assigned_gifts[i].erase().

You need to rethink your design and your use of file scope variables. This latter point is particularly so within the context of recursive functions.

Upvotes: 1

Csq
Csq

Reputation: 5855

You make a copy of the deque in this statement:

assigned_g_col_ids = assigned_g[i]

and then you use the erase of the original deque with and iterator from the copy

assigned_g[i].erase(assigned_g_col_ids.begin() + position);

Your code is really hard to read. E.g. you should declare variables where you use them, not at the beginning of the code as globals! (And I did not spot the recursive call either. As pointed out by David Hammen, do not use global variables as locals in recursive functions! And in any function!)

Also, using the standard algorithms as suggested in the comments can improve readability.

And I still do not understand what your code is trying to do.

Upvotes: 1

Related Questions