Discussion :

[babar63]

Bonjour à tous,

J'ai développez il y a quelques temps une classe de smart pointer de type :

Code :

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

1
2
3
4
5
6
template <class T>
class CSmartPointer
{
        T   *m_Pointer;
        int *m_Counter;
};

J'ai déjà eu l'occasion de faire quelques tests, il "semblerait" qu'ils soient fonctionnels. J'ai pu lire que dans mon cas de figure seul les références circulaires empêchées les smarts pointers de fonctionner. Or j'ai découvert un bug plutôt "étrange" (tout du moins incompris de moi). Voilà le détail de ce qui se passe :

Code :

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

1
2
3
4
5
6
class CSceneManager{
     //...
     bool ReadScene(std::string const &Filename);
     bool ReadScene(CNodePtr Root);
     CNodePtr m_Root;
}

Il faut savoir que CNodePtr est donc de type CSmartPointer<CNode> (CNode n'étant pas important ici). m_Root est initialisé a NULL par défaut et le compteur à 1. Maitenant dans la fonction ReadScene :

Code :

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

1
2
3
4
5
6
7
8
9
bool CSceneManager::ReadScene(std::string const &Filename)
{
     //...
 
     // Load the scene :
     CNodePtr NodeTmp = Loader->LoadScene(Filename);  // compteur à 1!!
     bool b = ReadScene(NodeTmp);                     // compteur à 2
     return b;                                        // compteur à 0
}

Une petite explication s'impose, Loader est une instance de classe permettant de charger des scènes, la fonction LoadScene renvoie un CNodePtr correspondant à la scène chargée. Et enfin :

Code :

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

1
2
3
4
5
6
bool CSceneManager::ReadScene(CNodePtr Root)
{
     //...
     m_Root = Root;   //compteur à 3
     return true;
}

Résultat l'objet est détruit... et je bloc dessus depuis un bon moment déjà alors que ça à l'air idiot (mais bon c'est toujours comme ça)
Si vous avez besoin de plus de code (CSmartPointer, fonctions...) n'hésitez pas à demander...

Merci d'avance et bonne fin de journée

[Mat007]

Salut,

Plutôt que de perdre du temps à (mal) refaire la roue pourquoi tu n'utilises pas boost ?

MAT.

[babar63]

Plutôt que de perdre du temps à (mal) refaire la roue

Cette argument est souvent cité, personnellement je fais partie de ceux qui aiment "refaire la roue", la question n'est pas de la refaire (bien ou mal), je continuerai d'ailleurs d'utiliser boost, mais le but est d'apprendre... ce n'est donc pas une perte de temps à mon avis (mais bon après c'est personnel).

J'utilise ma classe dans ce projet afin de la tester tout simplement...

[Mat007]

Ok, mettons...

Il faudrait sans doute en effet le code de CSmartPointer pour savoir exactement ce qui se passe.

MAT.

[babar63]

Voila un code plus complet sur ma classe mais je ne vois toujours pas d'erreur

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
template <class T>
class CSmartPointer
{
public:
 
	// Constructor :
	CSmartPointer(T *Pointer = NULL);
	CSmartPointer(CSmartPointer<T> const &SmartPointer);
 
	// Destructor :
	~CSmartPointer();
 
	// Operator :
	CSmartPointer<T> & operator = (CSmartPointer<T> const &SmartPointer);
	//...
 
	//....
 
private:
 
	T		*m_Pointer;			// Pointer on the object
	int		*m_Counter;			// Counter on the object pointed
};

Et le .inl mais toujours rien de compliqué :

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
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
//Constructor (1param) :
template <class T>
inline CSmartPointer<T>::CSmartPointer(T *Pointer)
{
	// Initialisation :
	m_Pointer = Pointer;
	m_Counter = new int(1);
}
 
//Copy constructor :
template <class T>
inline CSmartPointer<T>::CSmartPointer(CSmartPointer<T> const &SmartPointer)
{
	// Copy :
	m_Pointer = SmartPointer.m_Pointer;
	m_Counter = SmartPointer.m_Counter;
 
	// Incrementation :
	++(*m_Counter);
}
 
//Destructor :
template <class T>
inline CSmartPointer<T>::~CSmartPointer()
{
	// Decrementation :
	if( --(*m_Counter) == 0 )
	{
		delete m_Pointer;
		delete m_Counter;
	}
}
 
//Operator = :
template <class T>
inline CSmartPointer<T> & CSmartPointer<T>::operator = (CSmartPointer<T> const &SmartPointer)
{
	Assert( (this != &SmartPointer) );
 
	// Decrementation :
	if( --(*m_Counter) == 0 )
	{
		delete m_Pointer;
		delete m_Counter;
	}
 
	// Copy :
	m_Pointer = SmartPointer.m_Pointer;
	m_Counter = SmartPointer.m_Counter;
 
	// Incrementation :
	++(*m_Counter);
 
	return *this;
}

Je passe ici les détails sur les opérateurs de casts, prédicat etc...

Mais ce qui me surprend le plus c'est à ce niveau la :

Code :

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

CNodePtr NodeTmp = Loader->LoadScene(Filename);  // compteur à 1!!

Le compteur devrait être à deux (je suppose) mais je ne trouve pas l'erreur

[vdumont]

Tu peux montrer un main et ton Loader pour qu'on voit un cas typique d'utilisation?


Note:

Code :

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

1
2
3
	// Copy :
	m_Pointer = SmartPointer.m_Pointer;
	m_Counter = SmartPointer.m_Counter;

Ici j'espère que t'es conscient que tu ne fais pas une copy véritable mais tu fais seulement pointer tes pointeurs sur les mêmes addresses que SmartPointer ?

(Aussi, ourquoi utiliser un pointeur sur un entier au lieu de tout simplement utiliser un entier? Je vois pas l'intérêt dans ton cas.)

[Mat007]

Mais ce qui me surprend le plus c'est à ce niveau la :

Code :

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

CNodePtr NodeTmp = Loader->LoadScene(Filename);  // compteur à 1!!

Le compteur devrait être à deux (je suppose) mais je ne trouve pas l'erreur

Pourquoi à 2 ? Il n'y a plus qu'une référence, portée par NodeTmp, donc 1 me semble la bonne valeur. Ou j'ai raté un truc ?

Pourquoi utiliser un pointeur sur un entier au lieu de tout simplement utiliser un entier? Je vois pas l'intérêt dans ton cas.

Pour pouvoir partager le compteur entre toutes les copies du "smart pointer".

Sinon là comme ça en première lecture je ne vois trop rien qui puisse poser problème (enfin à part cet Assert qui n'a pas l'air terrible mais bon c'est pas le problème à la limite).

MAT.

[corrector]

Note:

Code :

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

1
2
3
	// Copy :
	m_Pointer = SmartPointer.m_Pointer;
	m_Counter = SmartPointer.m_Counter;

Ici j'espère que t'es conscient que tu ne fais pas une copy véritable mais tu fais seulement pointer tes pointeurs sur les mêmes adresses que SmartPointer ?

Je pense que :

1. il est conscient de copier les pointeurs
2. le comportement de la classe SmartPointer est modelé sur celui d'un pointeur C++
3. SmartPointer n'est pas un pointeur (c'est une classe), SmartPointer n'est pas très intelligente (c'est juste un pointeur et un compteur de référence manipulés ensemble), SmartPointer est donc ce qu'on appelle un smart-pointer

Comme c'est un pointeur à propriété partagée avec compteur de références, il vaudrait mieux l'appeler SharingPointer ou RefCountedPointer AMHA.

(Aussi, pourquoi utiliser un pointeur sur un entier au lieu de tout simplement utiliser un entier? Je vois pas l'intérêt dans ton cas.)

Parce que si chaque instance a une copie de l'entier ça ne risque pas de marcher!

[corrector]

Code :

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

1
2
3
4
5
6
7
template <class T>
class CSmartPointer
{
public:
 
	// Constructor :
	CSmartPointer(T *Pointer = NULL);

C'est volontairement que tu déclare une conversion implicite de T* vers CSmartPointer?

Si ce n'est pas le cas, ajoute le mot-clef "explicite".

Code :

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

1
2
3
4
5
6
7
template <class T>
inline CSmartPointer<T>::CSmartPointer(T *Pointer)
{
	// Initialisation :
	m_Pointer = Pointer;
	m_Counter = new int(1);
}

Il est idiomatique d'utiliser la liste d'initialisation, même quand ça ne change pas la signification du code, comme ici (les membres ont tous un type de base).

Code :

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

1
2
3
4
5
6
7
//Copy constructor :
template <class T>
inline CSmartPointer<T>::CSmartPointer(CSmartPointer<T> const &SmartPointer)
{
	// Copy :
	m_Pointer = SmartPointer.m_Pointer;
	m_Counter = SmartPointer.m_Counter;

Idem.

Code :

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

1
2
3
4
5
6
7
8
9
	// Incrementation :
	++(*m_Counter);
}
 
//Operator = :
template <class T>
inline CSmartPointer<T> & CSmartPointer<T>::operator = (CSmartPointer<T> const &SmartPointer)
{
	Assert( (this != &SmartPointer) );

Pourquoi assert? Tu veux interdire l'auto-affectation?

Mais ce qui me surprend le plus c'est à ce niveau la :

Code :

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

CNodePtr NodeTmp = Loader->LoadScene(Filename);  // compteur à 1!!

Le compteur devrait être à deux (je suppose) mais je ne trouve pas l'erreur

Des détails sur LoadScene?

[Emmanuel Deloget]

Pourquoi assert? Tu veux interdire l'auto-affectation?

Pour le coup, il vaut mieux - car la fonction commence par décrémenter le compteur avant de le réinitialiser avec la nouvelle valeur. Ce qui est problématique d'ailleurs, parce que l'assert est viré si on quitte le mode debug. Il faut un test - pas un assert (d'autant plus que l'auto-affectation est un idiome valide; pas le peine de planter le programme pour ça):

Code :

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

1
2
3
4
5
 
template <class T>
inline CSmartPointer<T> & CSmartPointer<T>::operator = (CSmartPointer<T> const &SmartPointer)
{
	if (&SmartPointer == this) return *this;

Pour comprendre ton problème, met un point d'arrêt sur chaque décrémentation du compteur, et vérifie la stack d'appel au moment ou tu tombe sur le point d'arrêt. Tu sauras alors qui tente de détruire le smart pointer, et tu auras plus de billes pour comprendre ce qui se passe.

[babar63]

Merci pour vos réponses,

Tu peux montrer un main et ton Loader pour qu'on voit un cas typique d'utilisation

Pour le main je peux en effet mais je ne vois pas l'intérêt, puisque le main se contente d'appeler la fonction ReadScene(...). Peut être le loader mais je ne pense pas que le problème soit là...

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
30
31
32
33
34
// Initalisation :
	Document Doc;
	if( !Doc.Read( Filename.data() ) )
	{
		SET_ERROR_CODE(INVALID_PARAMETER);
		return CNodePtr();
	}
 
	// Obtain the root element("scene") :
	Element RootElem = Doc.RootElement();
 
	// Root node :
	CSeparator * RootNode = new CSeparator();
 
	// Map for the nodes :
	std::map<std::string, CNodePtr> NodesMap;
 
	// Extract nodes :
	if( !LoadCameras(RootElem,NodesMap) ) return CNodePtr();
	if( !LoadLights(RootElem,NodesMap) ) return CNodePtr();
	if( !LoadMaterials(RootElem,NodesMap) ) return CNodePtr();
	if( !LoadTransforms(RootElem,NodesMap) ) return CNodePtr();
	if( !LoadModels(RootElem,NodesMap) ) return CNodePtr();
 
	// Extract scene graph :
	Element GraphElem = RootElem.FirstChild( "graph" );
	std::list<Element> NodeList = GraphElem.Children( "node" );
	for(std::list<Element>::iterator iter=NodeList.begin(); iter!=NodeList.end(); ++iter)
	{
		if( !LoadChild(RootNode,(*iter),NodesMap) ) return CNodePtr();
	}
 
	// Scene loaded :
	return RootNode;

Si quelque chose n'est pas clair dans ce code n'hésitez pas à demander.

Pourquoi à 2 ? Il n'y a plus qu'une référence, portée par NodeTmp, donc 1 me semble la bonne valeur

La fonction pour loader renvoyant un CNodePtr je pensais qu'un objet temporaire était crée... plus l'opérateur = ça aurait fait 2. En tout cas ce qui est sûr c'est que deux destructeurs sont appelés a la fin de la fonction (vérifié au debug).

C'est volontairement que tu déclare une conversion implicite de T* vers CSmartPointer

Oui en effet c'est volontaire pourquoi ce n'est pas bon?

Il est idiomatique d'utiliser la liste d'initialisation, même quand ça ne change pas la signification du code, comme ici

La en revanche ce n'est pas volontaire, j'ai oublié

Pour comprendre ton problème, met un point d'arrêt sur chaque décrémentation du compteur, et vérifie la stack d'appel au moment ou tu tombe sur le point d'arrêt. Tu sauras alors qui tente de détruire le smart pointer, et tu auras plus de billes pour comprendre ce qui se passe

Justement je l'ai fait! Deux destructeurs sont appelé à la fin de ma fonction :

Code :

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

bool CSceneManager::ReadScene(std::string const &Filename)

Le compteur atteint 0, l'objet est détruit alors qu'il devrait être stocké dans m_Root...

Je m'excuse d'avance pour le code, si il n'est pas clair. Le projet étant suffisamment important pour que je ne veuille pas vous décourager en affichant tout le code (ce qui en plus n'avancera pas forcément les choses). En tout cas merci pour votre aide

[babar63]

J'ai trouvé un exemple qui marche parfaitement (mais ça ne résout pas mon problème) :

Code :

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

1
2
3
4
5
6
7
8
9
bool CSceneManager::ReadScene(std::string const &Filename)
{
	//...
 
	// Load the scene :
	CNodePtr NodeTmp(new CSeparator());//Loader->LoadScene(Filename);       //Compteur à 1
	bool b = ReadScene(NodeTmp);                                            //Compteur à 2
	return b;                                                               //Compteur à 1
}

Les commentaires sur les compteurs représentent le nombre de références pointées APRES la ligne commentée. Un seul destructeur est appelé ici à la fin de ma fonction (NodeTmp) donc aucun problème... l'objet n'est pas détruit

[Médinoc]

Si ReadScene() prend un CNodePtr en paramètre, le destructeur de CNodePtr devrait être appelé deux fois: Une fois dans la fonction ReadScene(), une fois en fin de bloc.

Si tu veux hâter la destruction, tu peux faire ceci pour essayer de mieux déboguer:

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
bool CSceneManager::ReadScene(std::string const &Filename)
{
	//...
	bool b;
 
	// Load the scene :
	{
		CNodePtr NodeTmp(new CSeparator());//Loader->LoadScene(Filename);      //Compteur à 1
		b = ReadScene(NodeTmp);                                                //Compteur à 2
	}// Compteur à zéro, tout détruit.
	return b;
}

[Emmanuel Deloget]

Je viens de penser à quelque chose : ton opérateur=() est incorrect, car il ne règle pas le problème de l'assignation à soi-même. Tu dois aussi vérifier que pointeur et compteur ne pointent pas au même endroit.

De manière plus générale, tu devrais inversion les deux opérations (decr, delete) et (affectation, incr):

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
 
template <class T>
inline CSmartPointer<T> & CSmartPointer<T>::operator = (CSmartPointer<T> const &SmartPointer)
{
  int* old_counter = m_Counter;
  T* old_ptr = m_Pointer;
  m_Counter = SmartPointer.m_Counter;
  m_Pointer = SmartPointer.m_Pointer;
  ++(*m_Counter);
  if( --(*old_counter) <= 0 )
  {
    delete old_ptr;
    delete old_counter;
  }
  return *this;
}

De cette manière, tu t'affranchis des problèmes liés à l'auto-affectation.

Note: ton Root viens de l'objet Doc que tu crée dans ReadScene. Il sera détruit en sortant de cette fonction. (1 objet dans Doc, un object dans ReadScene; ces deux instances ont une portée automatique).

[Médinoc]

Je trouve cet opérateur = un peu compliqué.
Un copy-swap poserait sans doute moins de problèmes...

[corrector]

Je viens de penser à quelque chose : ton opérateur=() est incorrect, car il ne règle pas le problème de l'assignation à soi-même. Tu dois aussi vérifier que pointeur et compteur ne pointent pas au même endroit.

Tu veux dire qu'ajouter le test d'auto-affectation suivant à son operator= n'est pas suffisant?

Code :

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

1
2
if (this == &rhs)
    return;

Il me semble qu'il est suffisant à protéger son code. Peux-tu montrer un exemple qui pose problème?

De manière plus générale, tu devrais inversion les deux opérations (decr, delete) et (affectation, incr):

Oui, c'est la meilleur solution (plus générale).

[babar63]

Si tu veux hâter la destruction, tu peux faire ceci pour essayer de mieux déboguer:

Merci, en effet ça m'a aidé, l'erreur ne se trouve pas du tout ici... j'ai confondu le destructeur de "Loader" (qui est de type CSmartPointer<ILoader>) avec celui de "NodeTmp"

Voila un exemple de ce qui plantait :

Code :

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

1
2
3
4
5
6
7
8
9
10
class CSceneManager{
     //...
     bool AddCamera(CCameraPtr Camera);
     //...
     bool ReadScene(std::string const &Filename);
     bool ReadScene(CNodePtr Root);
     CNodePtr m_Root;
     TScenePtr m_Scene;
     std::vector<CCameraPtr> m_Cameras;
}

m_Scene et m_Cameras sont remplis lors de la lecture de mon graphe de scène via une fonction virtuelle pure :

Code :

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

virtual void Update(CSceneManager &SceneManager) = 0;

Maintenant voila la fonction Update redefinis pour une camera :

Code :

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

1
2
3
4
void CCamera::Update(CSceneManager &SceneManager)
{
	SceneManager.AddCamera(this);
}

La caméra est donc déjà pointé par CNodePtr, lorsque j'appelle AddCamera(this), j'appelle le constructeur de CSmartPointer et définit le compteur à 1 alors qu'il devrait être à deux....
La seule solution que j'ai trouvé :

Code :

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

1
2
3
4
void CCamera::Update(CSceneManager &SceneManager)
{
	SceneManager.AddCamera( new CCamera(*this) );
}

Je viens de penser à quelque chose : ton opérateur=() est incorrect, car il ne règle pas le problème de l'assignation à soi-même

Justement c'est la raison pour laquelle j'avais un assert, je voulais simplement interdire l'auto-affectation sans surcharger avec un test... D'ailleurs je ne comprend pas vraiment pourquoi utiliser un test pour éviter l'auto-affectation dans l'operateur =, celle-ci a-t-elle une signification? (moi je ne voit pas l'utilité )
Encore merci pour votre aide

[vdumont]

Justement c'est la raison pour laquelle j'avais un assert, je voulais simplement interdire l'auto-affectation sans surcharger avec un test... D'ailleurs je ne comprend pas vraiment pourquoi utiliser un test pour éviter l'auto-affectation dans l'operateur =, celle-ci a-t-elle une signification? (moi je ne voit pas l'utilité )
Encore merci pour votre aide

Parce qu'il faut que tu fasses attention aux programmeurs qui pourrait utiliser ta classe. Exemple typique:

Code :

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

1
2
3
4
 
Class Widget {...}
Widget w;
w = w; // stupide mais légal

Des fois c'est subtile et involontaire:

Code :

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

*px = *py; // self-assignment si les 2 pointent au même endroit

[babar63]

Oui mais dans ce cas pourquoi ne pas laisser l'assert, pour justement prévenir d'une telle faute? Ce qui me gène c'est de laisser un test JUSTE pour prévenir les futures fautes de programmations

[vdumont]

Si ma mémoire est bonne l'assertion va halter l'exécution de ton programme dans le cas d'une auto-affectation. Si c'est ce que tu veux d'accord, mais sinon le programme peut très bien continuer son exécution normale en gérant l'auto-affectation en retournant *this.