Problème de pointeur

Michael wrote:

Bonjour à tous,

voici mon problème.

J'ai les classes (simplifiées) suivantes:

En encore plus simplifié :

//----------------------------------------------------------------------
class DS_CrossBar_Pin
{
public:
DS_CrossBar_Pin();
};

//-----------------------------------------------------------------------
class DS_CrossBar_Input_Pin : public DS_CrossBar_Pin
{
public:
DS_CrossBar_Input_Pin *related_in;
DS_CrossBar_Input_Pin();
};

//-----------------------------------------------------------------------
class DS_CrossBar_Output_Pin : public DS_CrossBar_Pin
{
std::vector<DS_CrossBar_Input_Pin *> routing_list;
public:
DS_CrossBar_Input_Pin *routed_in;
DS_CrossBar_Output_Pin *related_out;
};


//-----------------------------------------------------------------------
class DS_CrossBar_Infos
{
public:
std::vector<DS_CrossBar_Output_Pin> output_pins;
std::vector<DS_CrossBar_Input_Pin> input_pins;

DS_CrossBar_Infos();
~DS_CrossBar_Infos();
void Get_Pins();
void Get_Relations_Pins();
};
//----------------------------------------------------------------------

Dans DS_CrossBar_Infos::Get_Pins() je remplis les vecteurs output_pins et
input_pins à coups de push_back:

output_pins.push_back(DS_CrossBar_Output_Pin(//...))
input_pins.push_back(DS_CrossBar_Input_Pin(//...))

Là, tu remplis des std:vector avec des *instances* de DS_CrossBar_Pin.
Ca suppose que tu peux manipuler ces instances par *valeur*, ce qui
n'apparaît absolument pas dans ton code. Ecrire des constructeurs de
copie et des opérateurs d'assignation rendrait cela plus explicite (ou
au moins mettre signaler cette propriété quelque part). Cela signifie
aussi que les adresses des instances contenues par les vectors sont
susceptibles de changer avec les vector (par jeu de réallocation), alors
comme tu joues avec ces adresses par la suite...

Honnêtement, ce serait beaucoup moins casse-gueule de remplir tes
vectors avec du boost::shared_ptr<> (http://www.boost.org).

Ensuite dans la fonction DS_CrossBar_Infos::Get_Relations_Pins() je
renseigne les membres *related_in, *routed_in...

Pour cela, je fais comme suit:

for (std::vector<DS_CrossBar_Input_Pin>::iterator input_pin =
input_pins.begin(); input_pin != input_pins.end(); ++input_pin)
{
//blabla
std::vector<DS_CrossBar_Input_Pin>::iterator ite = std::find
(input_pins.begin(),input_pins.end(),lRelated);
if (ite != input_pins.end())
input_pin->related_in = ite;
}
Comme vous le voyez j'attribue l'iterateur au pointeur related_in...
Ca compile, mais à l'execution j'ai une erreur...

Ca compile parce que ta STL implémente les itérateurs de std::vector
avec des pointeurs. Tu devrais écrire :

input_pin->related_in = &(*ite);

J'imagine que ce n'est pas la bonne affectation, mais je ne vois pas
comment faire...

Tu fais peut-être la bonne affectation, le problème c'est que le
pointeur n'a probablement plus de sens au moment ou tu l'utilises.
std::vector invalide ses itérateurs sur deletion ou insertion
d'élements. En gros, quand tu modifies la taille d'un std::vector, il
peut réallouer les objets contenus et modifier leurs adresses.

Pour que ça marche, il faut t'interdire de modifier la taille (ou plus
généralement d'invoquer toute opération invalidant les itérateurs) du
vecteur après avoir pris l'adresse des éléments contenus.

Pour faire ça tu peux essayer de jouer avec "const" mais ça risque
d'être compliqué.

Plus simplement, fait en sorte que "input_pins" et "output_pins"
contiennent des pointeurs sur le instances manipulées, voire mieux des
boost::shared_ptr (ou tout smart-pointer manipulable par valeur).

Par ailleurs, tu devrais passer ces membres "private", et protéger un
peu plus tes invariants d'objets (et commencer par les exprimer clairement).

Patrick Mézard

Bon du coup j'ai changé mes vecteurs en:

std::vector<DS_CrossBar_Output_Pin *> output_pins;

J'insère comme suit:
output_pins.push_back(new DS_CrossBar_Output_Pin((name.Pos("Audio") == 0 ?
VIDEO : AUDIO),name,i));

et j'utilise comme ceci:

std::vector<DS_CrossBar_Input_Pin *>::iterator ite = std::find
(input_pins.begin(),input_pins.end(),lRelated);
if (ite != input_pins.end())
(*input_pin)->related_in = *ite;

Je n'ai plus qu'un seul problème:

class DS_CrossBar_Pin
{
private:
long num_pin;
public:
__fastcall DS_CrossBar_Pin();

bool __fastcall operator==(long _num_pin) const
{ return _num_pin == this->num_pin; }
};

Dans le code suivant:

long lRelated = 3;

std::vector<DS_CrossBar_Input_Pin *>::iterator ite = std::find
(input_pins.begin(),input_pins.end(),lRelated);

J'ai le message d'erreur suivant:
[C++ Erreur] _algobase.c(102): E2034 Impossible de convertir 'const long'
en 'DS_CrossBar_Input_Pin *'

Je pense qu'il faut que je passe par une fonction boost ou quelque chose du
genre, mais je sais pas faire ça...

Tu fais peut-être la bonne affectation, le problème c'est que le
pointeur n'a probablement plus de sens au moment ou tu l'utilises.
std::vector invalide ses itérateurs sur deletion ou insertion
d'élements. En gros, quand tu modifies la taille d'un std::vector, il
peut réallouer les objets contenus et modifier leurs adresses.

Pour que ça marche, il faut t'interdire de modifier la taille (ou plus
généralement d'invoquer toute opération invalidant les itérateurs) du
vecteur après avoir pris l'adresse des éléments contenus.

Pour faire ça tu peux essayer de jouer avec "const" mais ça risque
d'être compliqué.

De toute manière les vecteurs ne bougent plus après leur remplissage...

Par ailleurs, tu devrais passer ces membres "private", et protéger un
peu plus tes invariants d'objets (et commencer par les exprimer
clairement).

C'est à dire? Quels membres passer en private? Et c'est quoi des invariants
d'objets dans mon cas?

Patrick Mézard <patrick.mezard@ifrance.com> a écrit:
[...]
Belle réponse ...
--
Pierre

Patrick Mézard <patrick.mezard@ifrance.com> writes:

Michael wrote:

Bonjour à tous,
voici mon problème.
J'ai les classes (simplifiées) suivantes:

En encore plus simplifié :

[...]

Ça fait effectivement du bien. Mais j'aurais encore simplifié, si
j'avais à poster un tel code, de manière à imposer le moins de travail
possible aux autres contributeurs. Par exemple :

struct Pin {
Pin() ;
} ;

struct Input_Pin : Pin {
Input_Pin * related_in ;
Input_Pin() ;
} ;

struct Output_Pin : Pin {
Input_Pin * routed_in ;
Output_Pin * related_out ;
private:
std::vector< Input_Pin * > routing_list ;
} ;

struct Infos {
std::vector< Output_Pin > output_pins ;
std::vector< Input_Pin > input_pins ;

Infos() ;
~Infos() ;
void Get_Pins() ;
void Get_Relations_Pins() ;
} ;

--drkm

Michael wrote:

Bon du coup j'ai changé mes vecteurs en:

std::vector<DS_CrossBar_Output_Pin *> output_pins;

J'insère comme suit:
output_pins.push_back(new DS_CrossBar_Output_Pin((name.Pos("Audio") == 0 ?
VIDEO : AUDIO),name,i));

Ton std::vector contient maintenant des pointeurs sur des objets alloués
dynamiquement. Ca veut dire plusieurs choses :
- Il est responsable de leur destruction, en particulier il faudra
appeler "delete" sur les pointeurs avant de détruire le std::vector.
- Il y a des problème plus subtils dûs aux exceptions. Par exemple ton
push_back est équivalent au code suivant :

// ------
DS_CrossBar_Output_Pin ds* = new
DS_CrossBar_Output_Pin((name.Pos("Audio") == 0 ? VIDEO : AUDIO),name,i);

output_pins.push_back(ds);
// ------

Si "push_back" lance une exception (genre std::bad_alloc) tu as une
fuite de mémoire sur "ds". Ok, c'est très improbable ici, mais vaut
mieux considèrer ce genre de choses dès maintenant.

Encore une fois tu peux t'en sortir, localement en faisant de la RAII,
ou avec des smart-pointers.

et j'utilise comme ceci:

std::vector<DS_CrossBar_Input_Pin *>::iterator ite = std::find
(input_pins.begin(),input_pins.end(),lRelated);
if (ite != input_pins.end())
(*input_pin)->related_in = *ite;

Je n'ai plus qu'un seul problème:

class DS_CrossBar_Pin
{
private:
long num_pin;
public:
__fastcall DS_CrossBar_Pin();

bool __fastcall operator==(long _num_pin) const
{ return _num_pin == this->num_pin; }
};

Dans le code suivant:

long lRelated = 3;

std::vector<DS_CrossBar_Input_Pin *>::iterator ite = std::find
(input_pins.begin(),input_pins.end(),lRelated);

J'ai le message d'erreur suivant:
[C++ Erreur] _algobase.c(102): E2034 Impossible de convertir 'const long'
en 'DS_CrossBar_Input_Pin *'

Je pense qu'il faut que je passe par une fonction boost ou quelque chose du
genre, mais je sais pas faire ça...

std::find cherche un élément dans une séquence en le comparant avec le
troisième argument. Mais les éléments de ta séquence ne sont plus des
instances mais des pointeurs sur des instances de DS_CrossBar_Pin, donc
ton implémentation de "operator==" ne sera jamais appelée.

Tu peux utiliser std::find_if et un foncteur :

// ------
namespace
{

struct EqualDsPin
{
EqualDsPin(long pin): m_pin(pin) {}
inline bool operator(const DS_CrossBar_Input_Pin* ds) const
{
return ds->num_pin==pin;
}
private:
long pin;
};

} //namespace
// ------

puis dans ton code :

// ------
std::vector<DS_CrossBar_Input_Pin *>::iterator ite =

std::find_if(input_pins.begin(),
input_pins.end(),
EqualDsPin(lRelated));
// ------

Par ailleurs, tu devrais passer ces membres "private", et protéger un
peu plus tes invariants d'objets (et commencer par les exprimer
clairement).

C'est à dire? Quels membres passer en private? Et c'est quoi des invariants
d'objets dans mon cas?

J'en sais trop rien :-).
Ca dépend de l'utilisation que tu fais de tes objets. Mais bon, on a dit
qu'il ne fallait pas que tes vectors soient modifiables après la
construction de ton graphe de DS_CrossBar (parce que c'est plus ou moins
de ça qu'il s'agit non ?). Donc, il ne faut pas qu'on puisse les
modifier de l'extérieur, donc ils doivent être private. Ce serait encore
mieux si la génération du graphe se faisait dans le constructeur de
DS_CrossBar_Infos, parce que ça sert à ça un constructeur, à mettre en
place les invariants de objets.

Il y a plein d'autres choses dont il faut s'assurer.
DS_CrossBar_Input_Pin contient un pointeur vers d'autres
DS_CrossBar_Input_Pin. Ca paraitrait logique que ce pointeur pointe sur
une instance valide de DS_CrossBar_Input_Pin ou bien qu'il soit NULL.
Ceci est un invariant de DS_CrossBar_Input_Pin. Mais comme le pointeur
est un membre public, n'importe qui peut changer sa valeur, donc
potentiellement briser l'invariant. Le mieux serait de le passer
"private" et d'ajouter des accesseurs, *et de les documenter*. Genre :

// ------
class DS_CrossBar_Input_Pin : public DS_CrossBar_Pin
{
public:
DS_CrossBar_Input_Pin() : related_in(0) {}

// "in" doit être NULL ou bien pointeur sur une instance
// valide de DS_CrossBar_Input_Pin, qui le restera pendant
// toute la durée de vie de l'instance courante.
void set_in(DS_CrossBar_Input_Pin *in)
{
related_in = in;
}

// Retourne NULL ou un pointeur sur une instance existante
// de DS_CrossBar_Input_Pin.
DS_CrossBar_Input_Pin* get_in() const
{
return related_in;
}

private:
DS_CrossBar_Input_Pin *related_in;
};
// ------

Ca peut paraître débile comme ça, parce que la paire d'accesseurs
n'apporte rien en terme d'accès par rapport à un membre public, mais ça
a l'avantage d'exprimer clairement l'invariant.
Maintenant en étant plus fainéant, on pourrait documenter la classe en
exprimant la chose, et compter sur la rigueur des gens l'utilisant, mais
c'est plus "dangereux".

Patrick Mézard

Merci pour toutes tes réponses, très très constructives...

Tout fonctionne bien désormais, il me reste à implémenter les
boost::shared_ptr ainsi que la RAII pour l'insertion dans le vecteur.

Encore merci

Mike

PS: bien vu pour DirectShow!

Michael <michael@no.com> writes:

Bon du coup j'ai changé mes vecteurs en:

std::vector<DS_CrossBar_Output_Pin *> output_pins;

J'insère comme suit:
output_pins.push_back(new DS_CrossBar_Output_Pin((name.Pos("Audio") == 0 ?
VIDEO : AUDIO),name,i));

Beurk. Ce n'est pas très lisible, je trouve. Pourquoi ne pas
décomposer un peu cette instruction ? Quelque chose comme ceci est
quand même plus clair, non ? :

{
Type media_type( 0 == name.Pos( "Audio" ) ? VIDEO : AUDIO ) ;
DS_CrossBar_Output_Pin *
pin = new DS_CrossBar_Output_Pin( media_type , name , i ) ;
output_pins.push_back( pin ) ;
}

ou :

output_pins.push_back(
new DS_CrossBar_Output_Pin(
0 == name.Pos("Audio") ? VIDEO : AUDIO ,
name ,
i
)
) ;

ou encore (que je trouve finalement le plus clair) :

{
Type media_type( 0 == name.Pos( "Audio" ) ? VIDEO : AUDIO ) ;
output_pins.push_back(
new DS_CrossBar_Output_Pin( media_type , name , i )
) ;
}

et j'utilise comme ceci:

std::vector<DS_CrossBar_Input_Pin *>::iterator ite = std::find
(input_pins.begin(),input_pins.end(),lRelated);
if (ite != input_pins.end())
(*input_pin)->related_in = *ite;

Je n'ai plus qu'un seul problème:

class DS_CrossBar_Pin
{
private:
long num_pin;
public:
__fastcall DS_CrossBar_Pin();

bool __fastcall operator==(long _num_pin) const
{ return _num_pin == this->num_pin; }
};

Es-tu sûr d'avoir besoin de ces « __fastcall » ? Qu'est-ce que
c'est ?

Dans le code suivant:

long lRelated = 3;

std::vector<DS_CrossBar_Input_Pin *>::iterator ite = std::find
(input_pins.begin(),input_pins.end(),lRelated);

J'ai le message d'erreur suivant:
[C++ Erreur] _algobase.c(102): E2034 Impossible de convertir 'const long'
en 'DS_CrossBar_Input_Pin *'

Tu as des pointeurs, maintenant, dans ton vecteur. Il te faut donc
les déréférencer si tu veux te servir de la valeur qu'ils pointent.
Tu peux t'inspirer du code suivant :

#include <algorithm>
#include <vector>

struct S {
S( int i )
: myInt( i ) {
}
bool operator==( int i ) const {
return i == myInt ;
}
private:
int myInt ;
} ;

struct Compare {
Compare( int i )
: myValue( i ) {
}
bool operator()( S const * s ) const {
return * s == myValue ;
}
private:
int myValue ;
} ;

int main() {
typedef std::vector< S * > Vect ;
typedef Vect::iterator Iter ;

Vect v ;

for ( int i = 0 ; i < 10 ; ++ i ) {
v.push_back( new S( i ) ) ;
}

Iter found = std::find_if( v.begin() , v.end() , Compare( 5 ) ) ;

for ( Iter it = v.begin() , end = v.end() ; it != end ; ++ it ) {
delete * it ;
* it = 0 ;
}
v.clear() ;
}

Au fait, lorsque j'ai rédigé cet exemple, j'avais d'abord placé
`Compare' en tant que classe locale à `main()'. Et là, GCC me
répondait :

fclcxx11.cc:33: error: type `main()::Compare', composed from a
local class is not a valid template-argument

qui laisse entendre que l'on ne pourrait pas utiliser de type local à
une fonction comme argument de modèle. Est-ce le cas ? Si oui,
quelle est la motivation (ou la contrainte) derrière cela ?

--drkm

Es-tu sûr d'avoir besoin de ces « __fastcall » ? Qu'est-ce que
c'est ?

D'après la doc Borland:

Description

Le modificateur __fastcall permet de déclarer des fonctions attendant que
des paramètres soient transmis dans les registres. Les trois premiers
paramètres sont transmis (depuis la gauche vers la droite) dans EAX, EDX et
dans ECX, s'ils rentrent dans le registre. Les registres ne sont pas
utilisés si le paramètre est de type virgule flottante ou structure.

Toutes les fonctions membre de classe fiche doivent utiliser la convention
__fastcall.

Le compilateur traite cette convention d'appel comme un nouveau
spécificateur de langage, comme _cdecl et _pascal.

Les fonctions déclarées en utilisant _cdecl ou _pascal ne peuvent pas avoir
aussi les modificateurs _fastcall car ils utilisent la pile pour
transmettre des paramètres. De même, le modificateur __fastcall
ne peut pas être utilisé avec _export.

Le compilateur met en préfixe un signe at ("@") devant le nom de fonction
__fastcall. Ce préfixe s'applique à la fois aux noms de fonction C non
substantypés et aux noms de fonction C++ substantypés.

Pour l'implémentation de __fastcall en Microsoft VC++, reportez-vous à
__msfastcall et __msreturn.

Remarque : Le modificateur __fastcall est soumis au substantypage. Voir la
description de l'option -VC.

J'ai lu quelque part qu'il est conseillé de les mettre pour toutes les
fonctions, donc je le fais...

Ca a toujours marché tel quel, donc je laisse... ;-)

Encore une fois tu peux t'en sortir, localement en faisant de la RAII,
ou avec des smart-pointers.

Hello,

en ce moment je modifie tout ça pour ajouter les smart_ptr. Mon souci est
le suivant:

j'ai les classes suivantes:

struct A
{
A* related_in;
};

struct B
{
A* routed_in;
B* related_out;

std::vector<A*> r_list;
};


et le code suivant:

typedef std::vector<boost::smart_ptr<B> > liste_B;
liste_B vect_B;

for (liste_B::iterator ite = vect_B.begin(); ite != vect_B.end(); ++ite)
{
(*ite)->routed_in = //???
(*ite)->r_list.push_back(//???
}

Ma question est de savoir si pour A et B il faut que je laisse tels quels
et j'utilise la fonction get() de shared_ptr, ou bien si je transforme tous
mes pointeurs en boost::shared_ptr:

struct A
{
boost::shared_ptr<A> related_in;
};

Mais dans ce cas est-ce que toutes les libérations de mémoire se feront
correctement?

Pour m'expliquer autrement, j'ai toujours les classes:

struct A
{
A* related_in;
};

struct B
{
A* routed_in;
B* related_out;

std::vector<A*> r_list;
};

J'ai deux vecteurs:

std::vector<boost::shared_ptr<A> > vect_A;
std::vector<boost::shared_ptr<B> > vect_B;

Maintenant je dois faire pointer A::*related_in vers une instance de A de
vect_A. Seulement maintenant vect_A contient un shared_ptr<A>.

Question:

je laisse tel quel et je fais par exemple:

a.related_in = vect_B.begin().get();

ou bien alors:

struct A
{
boost::shared_ptr<A> related_in;
};

et ensuite

a.related_in = vect_B.begin()

Mais dans ce cas suis-je sûr que related_in pointe bien vers l'instance de
A voulue, et suis-je sûr que la libération de la mémoire est complète?

Merci d'avance!