Discussion :

[rolf34]

Bonjour à tous,

Après avoir créer l'objet Map dans mon vector, je désire récupérer simplement son nom via la méthode getMapName() (Map.h/Map.cpp) mais le programme plante.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
using namespace std;

[...]

string map_name = "";
vector<Map*> v_maps;

[...]

cout << "Give it a name: ";
cin >> map_name;
v_maps.push_back(new Map(map_name));
cout << "Object ''" << v_maps.back()->getMapName() << "' well created." << endl << endl; //L'executable plante ici

L'executable plante à la ligne 13: v_maps.back()->getMapName() avec ce message :



Je me rend alors sur le header vector ligne 97 :

Fichier vector - ligne 97 :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
 
        {// report error
	_DEBUG_ERROR("vector iterator not incrementable");
	}

Je pensais qu'avec <vector>.back() je pouvait récupérer le dernier élément du vector sans passer par une boucle avec incrémentation d'un itérateur.

Ou alors je fais fausse route...

Des idées ?

[Pyramidev]

Pour l'instant, je ne vois pas d'erreur dans ton code. Quand un std::vector est non vide (ce qui est forcément le cas après ton push_back()), on peut accéder au dernier élément avec back().

Après une recherche internet, j'ai trouvé des exemples de personnes ayant rencontré le même message d'erreur que toi, mais elles essayaient d'incrémenter un itérateur invalidé avec erase, ce qui n'a rien à voir avec le code que tu viens d'écrire.

Je n'ai pas de version récente de Visual C++ sur mon poste.

Arrives-tu à reproduire le bogue avec un exemple de bout de code simple où on voit toutes les définitions ?
Par exemple, peux-tu tester que le code suivant s'exécute correctement chez toi ?

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
#include <iostream>
#include <string>
#include <vector>
 
struct StdStringWrapper
{
	std::string m_data;
	explicit StdStringWrapper(const std::string& data) : m_data(data) {}
	std::string getData() const {return m_data;}
};
 
int main()
{
	std::string data = "Hello world!";
	StdStringWrapper wrapper(data);
	std::vector<StdStringWrapper*> vect;
	vect.push_back(&wrapper);
	std::cout << vect.back()->getData() << '\n';
	return 0;
}

Avec GCC 6.3.0, le code ci-dessus s'exécute sans problème.

A part ça, peux-tu afficher davantage de code de ton fichier vector ? Je ne connais pas l'implémentation sur Visual C++.

D'ailleurs, quelle version utilises-tu ? [Edit : Ah, je vois dans le chemin de ton fichier que c'est Visual Studio 2015. Oublie ma dernière question.]

[rolf34]

Bonsoir et merci de ta réponse.

Le code que tu as fourni fonctionne sans problème. Le "Hello World" s'affiche bien avec le back().
Bizzare.

Voila le fichier en question (en :

fichier vector :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
		{	// preincrement
 #if _ITERATOR_DEBUG_LEVEL == 2
		const auto _Mycont = static_cast<const _Myvec *>(this->_Getcont());
		if (_Mycont == 0
			|| _Ptr == nullptr_t{}
			|| _Mycont->_Mylast <= _Ptr)
			{	// report error
			_DEBUG_ERROR("vector iterator not incrementable");
			}

 #elif _ITERATOR_DEBUG_LEVEL == 1
		_SCL_SECURE_VALIDATE(_Ptr != _Tptr());
		const auto _Mycont = static_cast<const _Myvec *>(this->_Getcont());
		_SCL_SECURE_VALIDATE(_Mycont != 0);
		_SCL_SECURE_VALIDATE_RANGE(_Ptr < _Mycont->_Mylast);
 #endif /* _ITERATOR_DEBUG_LEVEL */

		++_Ptr;
		return (*this);
		}

Mon objectif étant de référencer de nouveaux objets de manière dynamique. Même avec plusieurs objets ce message apparaît.

Je serais de retour ce dimanche sur le forum. Je regarderais aussi de nouveau le code, bien que tout semble correct..

[JolyLoic]

Je ne vois pas de soucis non plus dans ton code, mais tu l'as tellement coupé que le soucis est peut-être ailleurs. Peux-tu nous envoyer un code réduit mais complet qui reproduise le soucis ?
À défaut, peux tu au moins nous envoyer la call stack au moment de l'erreur ?

[Pyramidev]

Je trouve étrange que la fonction std::vector::back() appelle une fonction de préincrémentation.
Avec GCC 6.3.0, std::vector::back() est défini ainsi :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
      reference
      back() _GLIBCXX_NOEXCEPT
      { return *(end() - 1); }

Ne serait-ce pas plutôt cette ligne qui crash ?

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

v_maps.push_back(new Map(map_name));

Avec GCC 6.3.0, std::vector::push_back() incrémente un itérateur de fin. C'est peut-être pareil sur Visual Studio.
Pour être fixé sur la ligne qui plante, il faut regarder l'état de la pile au moment du crash.
Ou alors, tu peux insérer des instructions bidons :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
int instruction_bidon_1 = 0;
int instruction_bidon_2 = 0;
v_maps.push_back(new Map(map_name));
int instruction_bidon_3 = 0;
int instruction_bidon_4 = 0;
cout << "Object ''" << v_maps.back()->getMapName() << "' well created." << endl << endl;
int instruction_bidon_5 = 0;
int instruction_bidon_6 = 0;

En outre, peut-être que le vecteur a été cassé en amont à cause d'un problème subtile de mémoire. Je plussoie le conseil de JolyLoic : « Peux-tu nous envoyer un code réduit mais complet qui reproduise le soucis ? ».

[rolf34]

Bonsoir à vous et encore merci pour vos réponses,


J'ai revu mon code ce soir et je crois que j'ai très mal utilisé le débugger... Le plantage survient en fait lorsque je purge le vector à la fin du programme qui n'était pas inclut plus haut...
je crois que cette journée en montagne m'a bien rafraîchie la tête...


Voila donc le code en entier, simplifié ET avec l'erreur reproduite (je ferais désormais ça les prochaines fois) :

main.cpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
#include <iostream>
#include <string>
#include <vector>
#include "src\map.hpp"

using namespace std;

int main()
	{
	string map_name = "";
	vector<Map*> v_maps;

	cout << "Give it a name: ";
	cin >> map_name;
	v_maps.push_back(new Map(map_name));
	cout << "Object ''" << v_maps.back()->getMapName() << "' well created." << endl << endl;

	//Delete all vector's data
	for (std::vector<Map*>::iterator it = v_maps.begin(); it != v_maps.end(); ++it) //c'est ici que ça plante, à la seconde itération quelque soit le nombre d'objets dans le vector
		{
		v_maps.erase(it); 
		}

	return 0;
	}

map.hpp

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
#include <iostream>
 
using namespace std;
 
class Map
{
public:
	Map::Map(string map_name)
		{
		setMapName(map_name);
		}
 
	Map::~Map()
	{
	}
 
	void setMapName(string map_name)
		{
		this->_map_name = map_name;
		}
 
	string getMapName()
		{
		return this->_map_name;
		}
 
private:
	string _map_name;
};

Du coup dans main.cpp il passe une première fois dans la boucle for pour supprimer l'objet (et il le supprime bien car v_maps.size() décrémente de 1 après le erase()) mais plante à la deuxième itération sur la ligne du for quelque soit le nombre d'objets.

J'ai du mal interpréter la chose, je vais regarder ça de plus près demain et vous tiendrai au courant.

[Bousk]

Un coup d'oeil rapide à la doc de vector::erase stipule bien que les itérateurs suivant la position supprimée, ainsi que sur la position elle-même, sont invalidés suite à son appel.

[Invité]

Bonsoir,

Lorsque tu fais v_maps.erase(it); ton pointeur it est invalidé étant donné que tu supprimes l'élément. Il te faut récupérer le retour de erase() que te donne l'itérateur suivant. Voir la FAQ à ce sujet.

Remarques :

• la directive using namespace est déconseillée, d'autant plus dans un .h ;
• ton code n'est pas exception safe, et tu oublies de plus de deleter chaque *it. Selon l'élément responsable de leur durée de vie, tu devrais considérer l'utilisation un std::vector de std::unique_ptrs ;
• getMapName() serait mieux sous la forme std::string const & getMapName() const.

[Pyramidev]

Ah, ben, finalement :

Après une recherche internet, j'ai trouvé des exemples de personnes ayant rencontré le même message d'erreur que toi, mais et elles essayaient d'incrémenter un itérateur invalidé avec erase, ce qui n'a rien à voir avec comme dans le code que tu viens d'écrire.

D'où les vertus de faire une recherche internet avec le message d'erreur en question.
http://stackoverflow.com/questions/3...-incrementable

[JolyLoic]

Il y a un problème plus fondamental que l'itérateur invalidé... Que crois-tu que fasse erase ? Il enlève l'élément du vecteur, et c'est tout. C'est à dire qu'il enlève le pointeur, mais ne fait rien sur l'objet pointé. Tu as donc une fuite de mémoire.
Ensuite, pourquoi vouloir vider le vecteur ? De toute façon, il va être détruit bien proprement, le vider ne sert à rien (si tu voulais le vider à un autre moment, le plus simple serait v.clear()). Ce qui servirait, c'est de détruire les objets pointés :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
int main()
	{
	string map_name = "";
	vector<Map*> v_maps;
 
	cout << "Give it a name: ";
	cin >> map_name;
	v_maps.push_back(new Map(map_name));
	cout << "Object ''" << v_maps.back()->getMapName() << "' well created." << endl << endl;
 
	//Delete all vector's content
	for (std::vector<Map*>::iterator it = v_maps.begin(); it != v_maps.end(); ++it) //c'est ici que ça plante, à la seconde itération quelque soit le nombre d'objets dans le vector
		{
		 delete *it;
		}
 
	return 0;
	}

Mais ça encore c'est trop fastidieux. Dans du vrai code avec un vecteur de pointeurs qui possède les objets pointés (qui est responsable de leur destruction), on utiliserait plutôt un vector<unique_ptr<Map>>, du coup, la machine fait tout le nettoyage pour nous.

Mais même ça, c'est encore un peu compliqué... La première question, c'est pourquoi des pointeurs ? Qu'est-ce que t'empêche d'avoir juste un vector<Map> ?

[rolf34]

Bonjour tous le monde,

OK c'est donc une fuite mémoire, je pensais que erase() supprimait aussi l'objet...

Mais ça encore c'est trop fastidieux.
Dans du vrai code avec un vecteur de pointeurs qui possède les objets pointés (qui est responsable de leur destruction),
on utiliserait plutôt un vector<unique_ptr<Map>>, du coup, la machine fait tout le nettoyage pour nous.
Mais même ça, c'est encore un peu compliqué... La première question, c'est pourquoi des pointeurs ? Qu'est-ce que t'empêche d'avoir juste un vector<Map> ?

Il me semble plus performant de stocker juste l'adresse de l'objet plutôt que l'objet lui même dans le vector.

Mais ce que je fais actuellement c'est que je déclare un pointeur d'un vecteur?
Parce l'idée est d'avoir un vecteur contenant des adresses d'objets.


@Winjerome : Oui je savais pour using namespace c'étais juste pour le test. Par contre oui j’inclurai bien entendu le const pour cette variable.
@Pyramidev : Effectivement j'ai du le manquer l'autre jour... Merci

[JolyLoic]

Qu'est-ce qui te fait penser qu'un vecteur d'objet est moins performant qu'un vecteur de pointeurs ? À part dans le cas où l'objet est coûteux à déplacer, ce qui ne devrait pas être le cas ici (mais voir plus bas...), c'est tout le contraire:
- Un vecteur de pointeur va faire plein d'allocations mémoire, c'est une des choses les plus coûteuses qui soient
- Lors du parcours, comme les données sont éparpillées en mémoire au lieu d'être au même endroit, toutes les stratégies de cache mémoire et de prefetch des processeurs vont ne servir à rien, et le parcourt du vecteur sera bien plus lent...

Le seul problème, c'est qu'actuellement, ta classe est relativement coûteuse à déplacer... Pourquoi ? Parce que tu as défini un destructeur inutile dans ta classe. Cette définition empêche la génération d'un constructeur de déplacement par défaut, et donc des classes comme vector vont utiliser la copie plutôt que le déplacement quand elles devront déplacer tes objets. C'est peut-être un peu complexe et prématuré comme explication par rapport à là où tu en es dans l'apprentissage du langage, aussi je t'invite à juste retenir la règle simplifiée :

Une classe (autre d'une classe de base de hiérarchie) ne devrait pas avoir de destructeur dans 99% des case. Si elle en a un vide, il faut l'enlever. Si elle en a un qui fait des choses utiles, c'est qu'on n'a pas utilisé les bonnes abstractions (par exemple des pointeurs intelligents) qui s'occupent de la mémoire à notre place. Si on est dans le 1% des cas, il faut alors se préoccuper d'autres règles, en particulier sur les constructeurs par copie et par déplacement.

[rolf34]

Bonjour JolyLoic,

Merci pour tes explications claires. Je pensais qu'il fallait gérer soi-même la destruction des objets.

Je viens de consulter cette doc, le compilateur se débrouille à créer les destructions qu'il faut à la fin des accolades où sont construits les objets. J'aurai du lire ça avant...

Du coup, ça sera une journée lecture pour moi.

[Bousk]

Tu veux dire que tu ne connaissais pas la notion de portée d'une variable ?!