Discussion :

[Bluespear]

Bonjour,

je suis en train d'évaluer Boost pour tester mon code. Seulement il semblerait que de vouloir utiliser Boost.Test avec des pointeurs "non boost" cause quelques problèmes:

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
#define BOOST_TEST_MAIN
#define BOOST_TEST_DYN_LINK
#define BOOST_TEST_MODULE "HNode Testing module"
#include <boost/test/unit_test.hpp>
#include "../HNode.h"
 
BOOST_AUTO_TEST_SUITE(HNodeTest)
 
 
 
BOOST_AUTO_TEST_CASE(testInit) {
   // boost::shared_ptr<HNode> n1
    HNode* n1 = NULL;
    HNode* n2 = NULL;
    HNode* n3 = NULL;
    BOOST_TEST_MESSAGE("Setup HNodes");
    BOOST_CHECK(n1 == NULL);
    BOOST_CHECK(n2 == NULL);
    BOOST_CHECK(n3 == NULL);
    n1 = new HNode('a', 0.004);
    n2 = new HNode('e', 0.29);
    n3 = new HNode(n1, n2);
 
    BOOST_CHECK(n1->probability() == 0.004);
    BOOST_CHECK(n3->probability() == 0.294);
    BOOST_CHECK(n2->probability() == 0.29);
    BOOST_CHECK(n1->symbol() == 'a');
    BOOST_TEST_MESSAGE("Done");
    delete (n1);
    delete (n2);
    delete (n3);
}
 
BOOST_AUTO_TEST_SUITE_END()

l'erreur retournée est:

Code :

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

1
2
3
4
5
6
#
Running 1 test case...
#
unknown location(0): fatal error in "testInit": memory access violation at address: 0x00000008: no mapping at fault address
#
TestHNode.cpp(27): last checkpoint

La cause de cette erreur est le delete (n1);

Aurais-je oublié quelque chose ? Sachant que n3 va utiliser les pointeurs n1 et n2 en interne et que la totalité du code fait abstraction des smart pointers, je me vois mal retoucher toutes mes sources pour utiliser Boost et les smart pointers.

N.B.: Le test se passe bien si j'enleve les deletes... mais ça me donne droit à un joli leak

[zul]

Avant d'accuser boost, je regarderai de plus près Hnode::~Hnode. Il est plus que probable qu'il fasse quelquechose d'incorrect. Peux-tu nous montrer le code associé / fournir un exemple minimal qu'on puisse compiler ?

[koala01]

Salut,

Il ne faut déjà pas mettre les pointeurs entre parenthèses lorsque l'on invoque delete dessus: Ce n'est donc pas

Code :

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

1
2
3
    delete (n1);
    delete (n2);
    delete (n3);

qu'il faut écrire, mais

Code :

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

1
2
3
    delete n1;
    delete n2;
    delete n3;

Dans le même ordre d'idée, les lignes

Code :

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

1
2
3
     BOOST_CHECK(n1 == NULL);
    BOOST_CHECK(n2 == NULL);
    BOOST_CHECK(n3 == NULL);

ne servent strictement à rien, vu que, à ce niveau là, les adresses assignées au pointeur sont d'office nulles

Enfin, avant d'accuser boost, il semble effectivement intéressant de voir ce que font le constructeur et le destructeur de HNode, car, si:

• le constructeur ne fait qu'assigner l'adresse des pointeurs transmis en paramètres au membres et que
• le destructeur invoque delete sur les membre

il est tout à fait logique qu'il y ait une double libération de la mémoire, au plus tard lors de l'invocation de delete n3, vu que la mémoire allouée à n1 et à n2 est libérée une fois à l'occasion du delete correspondant, une fois par le destructeur qui est invoqué à l'occasion du delete n3.

Les tests sont normalement utilisés pour apporter la preuve qu'il est impossible de trouver une erreur dans ce qui est fait mais encore faut il... créer des tests qui soient eux-même exempts d'erreurs

[Bluespear]

Bonjour,

merci pour vos réponses.
Je sais bien que les check ==NULL ne servent à rien vu que c'est fait 1 à 3 lignes au dessus
N'ayant jamais utilisé Boost je préfère tester sur des choses sûres (ça sera effacé du test quand j'aurai trouvé mon problème).

Le code HNode.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
HNode::HNode(unsigned char symbol, double probability) {
    this->lchild_ = NULL;
    this->rchild_ = NULL;
    this->symbol_ = symbol;
    this->probability_ = probability;
 
}
 
HNode::HNode(HNode* leftChild, HNode* rightChild) {
    this->lchild_ = leftChild;
    this->rchild_ = rightChild;
    this->probability_ = leftChild->probability() + rightChild->probability();
 
}
 
HNode::~HNode() {
    if(lchild_ != NULL){
    delete lchild_;
    lchild_=NULL;
    }
    if(rchild_ != NULL){
    delete rchild_;
    lchild_=NULL;
    }
}

et HNode.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
class HNode {
public:
    HNode(unsigned char symbol, double probability);
    HNode(HNode* leftChild, HNode* rightChild);
    virtual ~HNode();
    double probability();
    unsigned char symbol();
private:
    HNode* lchild_;
    HNode* rchild_;
    unsigned char symbol_;
    double probability_;
//...
 
};

J'ai également modifié le test:

Code :

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

1
2
3
4
5
6
7
8
 
//....
if(n1!=NULL)
    delete n1;
    if(n3!=NULL)
    delete n2;
    if(n3!=NULL)
    delete n3;

Désolé si j'ai donné l'impression d'accuser Boost ça n'est pas le cas.

[zul]

Au vu de ton code, le problème est celui signalé par koala. Tu va détruire 2 fois la mémoire lié à n1, et de même pour n2 (une fois dans le delete n1, et une fois dans le dtor de n3), ce qui est invalide, d'où l'erreur. Deux solutions globalement pour ce genre de problème : utiliser des pointeurs intelligents, ou faire explicitement une copie des nodes.

De plus, delete 0 est une expression valide ne faisant rien, donc ça ne sert à rien de vérifier la non-NULLité des pointeurs avant de les détruire. Par contre, dans Hnode::Hnode(Hnode*, Hnode*), il serait ici de bon goût de vérifier la non-NULLité des arguments avant de les déférencer.

Enfin, étant donné que tu manipule des pointeurs nus, il te faut définir correctement le constructeur par copie, et l'opérateur = (par défault, tu va recopier les pointeurs, et tu risque encore une fois de les delete N fois).

[koala01]

Bonjour,

merci pour vos réponses.
Je sais bien que les check ==NULL ne servent à rien vu que c'est fait 1 à 3 lignes au dessus
N'ayant jamais utilisé Boost je préfère tester sur des choses sûres (ça sera effacé du test quand j'aurai trouvé mon problème).

Pourquoi faire des choses inutiles

A partir du moment où tu sais pertinemment que le problème ne vient pas de là et où, d'autre part, un test unitaire est prévu pour mettre en évidence des problèmes dont on soupçonne qu'ils pourraient apparaitre cela n'a aucun sens

Le code HNode.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
HNode::HNode(unsigned char symbol, double probability) {
    this->lchild_ = NULL;
    this->rchild_ = NULL;
    this->symbol_ = symbol;
    this->probability_ = probability;
 
}
 
HNode::HNode(HNode* leftChild, HNode* rightChild) {
    this->lchild_ = leftChild;
    this->rchild_ = rightChild;
    this->probability_ = leftChild->probability() + rightChild->probability();
 
}
 
HNode::~HNode() {
    if(lchild_ != NULL){
    delete lchild_;
    lchild_=NULL;
    }
    if(rchild_ != NULL){
    delete rchild_;
    lchild_=NULL;
    }
}

et HNode.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
class HNode {
public:
    HNode(unsigned char symbol, double probability);
    HNode(HNode* leftChild, HNode* rightChild);
    virtual ~HNode();
    double probability();
    unsigned char symbol();
private:
    HNode* lchild_;
    HNode* rchild_;
    unsigned char symbol_;
    double probability_;
//...
 
};

J'ai également modifié le test:

Code :

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

1
2
3
4
5
6
7
8
 
//....
if(n1!=NULL)
    delete n1;
    if(n3!=NULL)
    delete n2;
    if(n3!=NULL)
    delete n3;

Ou la la... Il y a à dire là dessus

D'abord, comme l'a fait valoir zul, delete sur NULL est garantie sans effet.

Il ne sert donc à rien de placer un test avant de l'effectuer

Par contre, ce sur quoi tu ne semble pas avoir "percuté", c'est que ce n'est pas parce que tu mets les membres lchild_ et rchild_ à NULL (d'ailleurs, tu mets... lchild_ à null après avoir... détruit rchild_ ) que les variables transmises au constructeur (respectivement les pointeur n1 et n2) seront mises à jour et pointeront... sur NULL.

Un pointeur n'est finalement qu'une variable numérique un peu particulière en cela qu'elle contient... l'adresse mémoire à laquelle on va trouver l'objet réellement manipulé (l'objet "pointé").

Mais cela signifie qu'elle est donc soumise exactement au même règles que les variables classiques:

Lorsque tu transmet un pointeur à une fonction, il y a copie du pointeur (de l'adresse représentée par le pointeur), même s'il n'y a pas... copie de l'objet pointé.

Si, dans la fonction, tu décide de faire pointeur ce pointeur ailleurs, l'adresse qui lui sera assignée ne sera donc pas répercutée... sur la variable d'origine.

Un petit exemple pour t'en convaincre

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
#include <iostream>
void foo( int * ptr)
{
    std::cout<<"valeur de ptr dans foo"<<std::endl
             <<std::hex<<ptr<<std::endl;
    /* plaçons ptr à NULL */
    ptr = NULL;
 
    std::cout<<"quand il est mis a zero "<<std::endl
             <<std::hex<<ptr<<std::endl;
}
int main()
{
    int *tab = new int;
 
    std::cout<<"valeur de tab avant appel de foo"<<std::endl
             <<std::hex<<tab<<std::endl;
    foo(tab);
 
    std::cout<<"valeur de tab apres appel de foo"<<std::endl
             <<std::hex<<tab<<std::endl;
    delete tab; // travaillons proprement :D
    return 0;
}

va donner un résultat proche de
la valeur de tab avant appel de foo

Code :

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

1
2
3
4
5
6
7
0x2f7630
la valeur de ptr dans foo
0x2f7630
quand il est mis a zero
0
la valeur de tab apres appel de foo
0x2f7630

Les valeurs risquent de changer un tout petit peu selon l'état du système, mais tu remarque que tab n'est absolument pas influencé par la modification de l'adresse vers laquelle ptr pointe dans foo

Le problème est exactement identique dans ton code: n1 et n2 ne seront pas modifiés par le fait que n3->lchild_ et n3->rchild_ soient mis à NULL lorsque tu détruit n3, pas plus que n3->lchild_ ou n3->rchild_ ne seront modifié lorsque tu... détruira n1 et n2

Ce sont à chaque fois des variables clairement distinctes qui (manque de bol ) pointent vers des adresses identiques, et tout ton problème vient de là

Désolé si j'ai donné l'impression d'accuser Boost ça n'est pas le cas.

C'était d'autant plus injuste que ce n'était pas le cas

Au vu de ton code, le problème est celui signalé par koala. Tu va détruire 2 fois la mémoire lié à n1, et de même pour n2 (une fois dans le delete n1, et une fois dans le dtor de n3), ce qui est invalide, d'où l'erreur. Deux solutions globalement pour ce genre de problème : utiliser des pointeurs intelligents, ou faire explicitement une copie des nodes.

Une troisième solution est de se baser sur le fait que le destructeur fait bien son travail et de ne détruire explicitement que... le noeud qui sert de racine:

Je reprend le code de test et je le modifie:

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
BOOST_AUTO_TEST_CASE(testInit) {
   // boost::shared_ptr<HNode> n1
    HNode* n1 = NULL;
    HNode* n2 = NULL;
    HNode* n3 = NULL;
    BOOST_TEST_MESSAGE("Setup HNodes");
    /*supprimé lest checks inutiles ;) */
    n1 = new HNode('a', 0.004);
    n2 = new HNode('e', 0.29);
    n3 = new HNode(n1, n2);
 
    BOOST_CHECK(n1->probability() == 0.004);
    BOOST_CHECK(n3->probability() == 0.294);
    BOOST_CHECK(n2->probability() == 0.29);
    BOOST_CHECK(n1->symbol() == 'a');
    BOOST_TEST_MESSAGE("Done");
    /* le destructeur de n3 invoque delete sur n1 et n2...
     * pas besoin d'en faire plus :D
     */
    delete n3;
}

De plus, delete 0 est une expression valide ne faisant rien, donc ça ne sert à rien de vérifier la non-NULLité des pointeurs avant de les détruire. Par contre, dans Hnode::Hnode(Hnode*, Hnode*), il serait ici de bon goût de vérifier la non-NULLité des arguments avant de les déférencer.

Tout à fait

Enfin, étant donné que tu manipule des pointeurs nus, il te faut définir correctement le constructeur par copie, et l'opérateur = (par défault, tu va recopier les pointeurs, et tu risque encore une fois de les delete N fois).

humm... C'est à discuter...

Il faut, très clairement, faire quelque chose pour le constructeur par copie et pour l'opérateur d'affectation.

Mais cela ne veut pas *forcément* dire qu'il faut les redéfinir.

Il y a, en effet, deux solutions envisageables:
Soit les noeuds sont assignables et copiables, et il faut les redéfinir (en utilisant de préférence l'idiôme copy 'n swap)
Soit il n'y a aucun sens à permettre leur copie et leur assignation (ce qui sera souvent le cas pour des noeuds car ils ont souvent sémantique d'entité), et, dans ce cas, il faut s'assurer que la copie et / ou l'affectation sera refusée.

Le moyen d'arriver à cette deuxième solution en attendant la nouvelle norme est de déclarer (sans les définir) le constructeur par copie et l'opérateur d'affectation dans la visibilité privée:

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
class HNode {
public:
    HNode(unsigned char symbol, double probability);
    HNode(HNode* leftChild, HNode* rightChild);
    virtual ~HNode();
    double probability();
    unsigned char symbol();
private:
    HNode* lchild_;
    HNode* rchild_;
    unsigned char symbol_;
    double probability_;
    // le constructeur par copie !!! déclaré mais non défini !!!
    HNode(HNode const &);
    // l'opérateur d'affectation !!! déclaré mais non défini !!!
    HNode & operator =(HNode const & );
//...
 
};

Ce sera plus simple avec la nouvelle norme (C++11) car on dispose du moyen de dire qu'un des "big four" n'est pas à implémenter par le compilateur:

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
class HNode {
public:
    HNode(unsigned char symbol, double probability);
    HNode(HNode* leftChild, HNode* rightChild);
    // le constructeur par copie désactivé
    HNode(HNode const &) = delete;
    // l'opérateur d'affectation désactivé
    HNode & operator =(HNode const & ) = delete ;
    virtual ~HNode();
    double probability();
    unsigned char symbol();
private:
    HNode* lchild_;
    HNode* rchild_;
    unsigned char symbol_;
    double probability_;
//...
 
};

[Bluespear]

Merci beaucoup à tous, en particulier koala01 pour l'explication détaillée.
Je comprends mieux les bétises que j'ai fait dans mon code ... dur dur en venant du monde Java