Discussion :

[tonio_a_588]

Bonjour,
j'ai un problème qui me taraude l'esprit depuis quelques heures, sans que j'arrive à le résoudre ;
Carte est une classe qui a comme attribut un objet de classe Proj
Proj a un attribut de type char**
Le constructeur de Carte prend en argument un objet de classe Proj qui serta initialiser l'attribut de Carte
Pour ce faire, je dois definir l'operateur= ; que faire ? j'ai le bout de code suivant qui conduit à une erreur de segmentation

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
#include <iostream>
#include <malloc.h>
using namespace std;
 
class Proj
{
public:
char **args;
int nb_args;
string nom;
Proj(){nb_args=1;
args=reinterpret_cast <char**>(malloc(sizeof(char*)*nb_args));
sprintf(args[0],"%s","essai");
nom="lambi";
}
Proj (Proj &prj);
Proj operator=(Proj &prj);
~Proj(){free(args);}
 
};
 
class Carte
{
public:
Proj lamb;
int a;
Carte(int aa, Proj prj){a=aa;lamb=prj;} // here is my problem : comment 
//definir mon operateur = pour Proj ? je ne peux pas utiliser mon copie constructeur...
~Carte(){};
};
 
 
Proj::Proj(Proj &prj) // constructeur par copie (a priori inutilise)
{
nb_args=prj.nb_args;
args=reinterpret_cast<char**>(malloc(sizeof(char*)*nb_args));
for(int i=0; i<nb_args;i++)
args[i]=prj.args[i];
}
 
Proj Proj::operator=(Proj &prj)
{
nb_args=prj.nb_args;
// mais je ne peux pas faire de malloc ici quand même !!
// Alors comment copier prj.args dans args ? sachant qu'ils 
// n'ont même pas forcément la même taille ?
}
 
main()
{
Proj exemple_proj;
Carte(4,exemple_proj);
}

[Florian Goo]

Bonjour,

Code :

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

1
2
 
args=reinterpret_cast <char**>(malloc(sizeof(char*)*nb_args));

Ouh la vache… avec une horreur pareille, ça m'étonne pas que ça finisse avec une erreur de segmentation !

Préfère l'utilisation de std::string (à la place de char*) et de std::vector (à la place des tableaux). En l'occurrence, utilise un std::vector<std::string> plutôt que char**.
Oublie malloc et free, utilise new.
Oublie sprintf.
Indente ton code.
Donne des noms plus évocateurs à tes variables et à tes classes (Carte, ok, mais Proj, qu'est ce que c'est ?).

Bon courage

[tonio_a_588]

Merci de ta réponse,c'est vrai que c'est moche, mais je suis contraint d'utiliser un char** pour utilser une fonction d'une bibliothèque C de projection (d'où Proj).
Je vais essayer d'ecrire un truc plus clair.
Merci de tes conseils

[Florian Goo]

Utilise tout de même des string et vector au moins dans tes interfaces. Ce n'est pas parce que tu utilises une lib en C que ça doit venir pourrir tout ton code ! Et puis, si un jour tu venais à remplacer cette lib par une lib C++, tu te retrouverais tout bête avec tes char** partout.

Ensuite, si c'est possible, écris une bonne fois pour toute une fonction de conversion std::vector<std::string> vers char**, ce sera plus lisible et plus propre.

Nomme ta classe Projection, dans ce cas… c'est suffisamment compliqué de relire un code pour se payer le luxe de mettre des abréviations partout .

Enfin, et surtout, n'emploie pas reinterpret_cast<> à la légère sans savoir ce que tu fais (du genre « tiens, ça veut pas compiler sauf si j'écris reinterpret_cast, alors je vais utiliser ça »). C'est très, TRÈS dangereux !

[Florian Goo]

Désolé, j'ai écrit une bêtise tout à l'heure (j'ai édité) :

Code :

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

1
2
Carte(int aa, Proj prj){a=aa;lamb=prj;} // here is my problem : comment 
//definir mon operateur = pour Proj ? je ne peux pas utiliser mon copie constructeur...

Dans ce cas, c'est bien operator= qui est utilisé (et non pas le constructeur par copie).

Cependant, il faut que tu utilises les listes d'initialisation pour faire ça proprement :

Code :

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

1
2
3
4
5
6
 
Carte(int aa, const Proj& prj):
    a(aa),
    lamb(prj)
{
}

Et effectivement, dans ce cas, c'est le constructeur par copie qui est utilisé.
Remarque également que si tu souhaites faire une copie de prj, il est préférable que tu transmettes prj par référence constante plutôt que par valeur (regarde le code).

[tonio_a_588]

Super, Merci de ces conseils, je vais essayer d'arranger tout ça, mais j'y vois déjà plus clair!

[3DArchi]

Salut,
En plus de tous les conseils avisé de maître Florian, je rajouterais qu'une bonne implémentation de l'opérateur égal est:

Code :

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

1
2
3
4
5
6
7
 
MaClasse&operator=(const MaClasse&_rhs)
{
    MaClasse l_tmp(_rhs);
    swap(this,l_tmp);
    return *this;
}

Dans swap tu fais l'échange des valeurs et des pointeurs si hélas tu en as.

[Médinoc]

En fait, suite à des conseils plus récents, la bonne méthode serait plutôt cela:

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
//MaClasse.cpp
#include "MaClasse.hpp"
 
MaClasse& MaClasse::operator=(MaClasse tmp) //par valeur
{
	Swap(tmp);
	return *this;
}
 
void Swap(MaClasse &other)
{
	std::swap(m_pointeur, other.m_pointeur);
	// etc.
}

Si j'ai bien compris, prendre l'argument par valeur permet d'éviter une copie inutile si l'argument de = est un temporaire.

PS: Et je joins le header, au passage.

Code C++ :

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
// MaClasse.hpp
#include <algorithm>
 
class MaClasse
{
public:
	//Constructeur, constructeur de copie, destructeur
	MaClasse( /*...*/ );
	MaClasse(MaClasse const &src);
	~MaClasse();
 
	//Opérateur d'affectation et fonction de swap
	MaClasse& operator=(MaClasse tmp); //par valeur
	void Swap(MaClasse & other);
private:
	int * m_pointeur;
};
 
//Spécialisation de std::swap
namespace std
{
 
template<>
inline void swap(MaClasse &left, MaClasse &right)
{
	left.Swap(right);
}
 
}// namespace std

[Jean-Marc.Bourguet]

En passant, 3DArchi, attention a ne pas utiliser le swap de std (sauf si on l'a specialise explicitement).

[Médinoc]

En effet, car ta pile exploserait (le swap de std, non-spécialisé, fait simplement une copie et deux affectations)...

[3DArchi]

Si j'ai bien compris, prendre l'argument par valeur permet d'éviter une copie inutile si l'argument de = est un temporaire.

OK, argument retenu!

En passant, 3DArchi, attention a ne pas utiliser le swap de std (sauf si on l'a specialise explicitement).

Effectivement, j'avais oublié de le préciser .

[Goten]

juste une question alors :

l'opérateur d'affectation tel qu'il est codé dans la faq est encore améliorable?

[Ulmo]

La solution que j'utilise n'est pas citée ici. Pose-t-elle un problème auquel je n'ai pas pensé ?

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
MyClass::MyClass(const Myclass &mc) {
    [tous les pointeurs à NULL];
    clone(mc);
}
 
MyClass &MyClass::operator=(const Myclass &mc) {
    if (this != &mc)
        clone(mc);
    return *this;
}
 
void MyClass::clone(const MyClass &mc) {
    delete [pointeurs];
    [pointeurs] = new [gnagnagna];
    [copier les valeurs];
}

[Médinoc]

C'est un gaspillage, car tu fais deux initialisations (voire plus) dans le constructeur de copie. Ta stratégie risque de ne pas marcher si ta classe contient des champs complexes qui interdisent certaines modifications.

De plus, ce n'est pas exception-safe, contrairement à l'idiome copy-and-swap.