Debug assertion failed: Vectors iterator incompatible

[anarelle]

I'm using VS2013 to work on this program. I'm not using OOP (I'm not coding any classes) so my objects are just structs.
There's a struct holding data about a book (name, genre and isbn of a book):

struct t_libro
{
	string nombre;
	string genero;
	long isbn;
};

I read the necessary info from keyboard and store it in a text file. So far so good, that works.
Then I want to allow the user to delete a book from the file. So I first load all the file contents into a vector<libro> (which I've called "coleccion_libros"), and this also works ok.
I ask the user which book to delete, and allow them to search by name ("nombre_buscado"). I do the required changes (i.e.: delete a book) in the vector, which I finally serialize again into the text file, replacing the previous file contents.

auto iterador = coleccion_libros.begin();
while (iterador != coleccion_libros.end())  
		if (iterador->nombre == nombre_buscado)  //nombre_buscado holds the book name user is searching for
		{
			eliminar_libro(iterador, coleccion_libros);
		}
		else
			++iterador;
	if (iterador == coleccion_libros.end())
		cout << "\n\n\***BOOK NOT FOUND***";
}

//Once the vector is modified, erase file contents so the new vector can be saved
ofstream archivo("libros.txt", ios::trunc);
archivo.close();

//Serialize vector contents into file
for (auto libro : coleccion_libros)
	serializar_libro("libros.txt", libro);

This is my delete function:

void eliminar_libro(vector<t_libro>::iterator iterador, vector<t_libro>& libros)
{
	char opcion;
	do {
		cout << "\n\nDO YOU WISH TO DELETE [" << iterador->nombre << "]? (Y)es/(N)o: ";
		cin >> opcion;
		cin.ignore();
		tolower(opcion);
	} while ((opcion != 'y') && (opcion != 'n'));
	if (opcion == 'y'){
		cout << "\n\n[ " << iterador->nombre << " ] ";
		iterador = libros.erase(iterador);
		cout << "***BOOK WAS REMOVED***";
	}
	else
		cout << "\n***NO DATA HAS BEEN ALTERED***";
}

I have a similar function to modify data, in which I pass the iterator as an argument in the same way as in the delete function. The modify function works perfectly.
But when it comes to deleting, I get a "vectors iterator incompatible" error: http://k46.kn3.net/8/8/2/3/D/6/D7A.jpg
This error is thrown right after the console outputs "***BOOK WAS REMOVED***".
I've read a few other threads about this but I don't see anything in common with my code so I'm a bit clueless on what could be going on.
Is this a Visual Studio bug, as I've read somewhere? If not, what am I doing wrong?
Thanks!!

[ne555]

erase() invalidates the iterator.
i notice that you do iterador = libros.erase(iterador); in eliminar_libro(), but there you passed iterator by copy, so you are modifying a local variable.


> This error is thrown right after the console outputs "***BOOK WAS REMOVED***".
Run in debug mode and it will point to the exact line that causes the error.


By the way

//Serialize vector contents into file
for (auto libro : coleccion_libros)
	serializar_libro("libros.txt", libro);

¿are you opening the file each time in serializar_libro()?

[anarelle]

Thanks so much for your answer :)

As for the first part: I passed the iterator by copy because I assumed I was passing a pointer to a vector element. So I just need the memory address of that vector element, not a pointer to that memory address. Am I analyzing this the wrong way?
And how can I erase the vector element pointed by the iterator and return the modified vector in the "eliminar_libro" function without getting this error message?

About the vector serialization, I am indeed opening the file each time. This is the function being called:

void serializar_libro(const string& file_name, t_libro& un_libro)
{
	ofstream archivo(file_name, ios::app);
	archivo << un_libro.nombre << endl;
	archivo << un_libro.genero << endl;
	archivo << un_libro.isbn << endl;
	archivo.close();
}

I use this function in other parts of the code to save just one book in my text file, and I need all file handling operations separated from the rest of the code.
Is it bad to open and close the file for every vector element? Should I use separate functions when I want to save to file just 1 struct and when I'm saving a whole bunch of them?
Again, thanks for the enlightment :)

[LB]

Because you assign a new value to the iterator you should pass it by reference. (It would be the same with pointers - you'd pass the pointer by reference)

[anarelle]

Ah, silly me! So now I'm passing the iterator by reference and it works in most cases, except when I try to erase the last element in the vector. Sounds logical to me, since I've read that erase() returns an iterator following the last removed element.
So I tried to trick it by doing:

iterador = --(libros.erase(iterador));

But of course it didn't work, hehe. I suppose it's because once the iterator was invalidated I can't decrement it.

So I tried with an if statement before erasing, to ask if iterator == vector.end() and in that case I used pop_back():

if (iterador == libros.end())
	libros.pop_back();
else
	iterador = libros.erase(iterador);

Is this an appropriate approach?

[LB]

anarelle wrote:

except when I try to erase the last element in the vector.

In that case it returns the end iterator, aka std::end(libros) - just check for that case in your calling code.

[ne555]

You shouldn't need a special case for the last element, try to provide a testcase http://www.eelis.net/iso-c++/testcase.xhtml

Just notice that you ask the user if they want to delete the book, however if they say 'no' then you won't advance in the vector.


> Is it bad to open and close the file for every vector element?
I would say that it is an expensive operation, was expecting that you leave the file open and pass it around void serializar_libro(std::ostream& file_name, t_libro& un_libro) (ostream so you can print it too)

But if it is too much trouble to modify the function and you don't notice the performance hit, then don't touch it.