ProjetM1MPRI2013 / central

Repo principal
6 stars 0 forks source link

Warning valgrind (dans test local/globalstate): position non initialisée #42

Closed dbaelde closed 10 years ago

dbaelde commented 10 years ago

Quand on crée un positionable, il fait un setX/setY pour régler sa position initiale. A ce moment là les valeurs de x et y sont non définies, donc on a un comportement non défini. Voici un patch ci-dessous, par contre il ne donnera pas exactement le même comportement: en général la valeur initiale de x/y sera 0, et on aura donc un évènement changeTile à l'initialisation si et seulement si la valeur initiale n'est pas 0. On peut vouloir toujours émettre changeTile, ou jamais (comme dans le patch actuel).

--- a/code/src/simulation/positionable.cc
+++ b/code/src/simulation/positionable.cc
@@ -25,9 +25,7 @@ Positionable::Positionable(float x,float y) : FogDisabler() 
 }

-Positionable::Positionable(Position& p) : FogDisabler() {
-  position.setX(p.getX());
-  position.setY(p.getY());
+Positionable::Positionable(Position& p) : FogDisabler(), position(p) {
   listen("Position::changedTile",position,&Positionable::changedTile);
   return;
 }

Edit: attention, la même modif doit être appliquée (au moins) au constructeur avec uuid.

dbaelde commented 10 years ago

Note pour réveiller des gens: même si l'erreur a été trouvée via un test de @kanunikov-denys le fichier incriminé appartient à @grosxor.

jlallemand1 commented 10 years ago

En effet, position n'est pas initialisée dans ces deux constructeurs (c'est bon pour les autres). Le patch en question résout bien le problème, mais je ne suis pas certain du comportement attendu pour l'évènement changedTile, @MrKulu qui utilise cet évènement doit le savoir

dbaelde commented 10 years ago

Dans le même fichier, le constructeur sans argument Position::Position() n'initialise pas vraiment la position. C'est cppcheck qui m'a mis la puce à l'oreille: ligne 10, un objet Position est crée puis immédiatemment détruit.

jlallemand1 commented 10 years ago

Après une enquête approfondie, le problème vient du commit c1d3e29 avant lequel le constructeur était comme celui du patch… Le comportement attendu est bien de ne générer aucun évènement. J'ai corrigé ça, j'ai aussi modifié le constructeur par défaut de position qui devrait marcher correctement.

dbaelde commented 10 years ago

Merci, cela semble bien.