Discussion :

[Caidriq]

Voila je cherche depuis quelques jours mais je ne trouve pas la solution. Ce code me renvoie un message "Erreur de segmentation (core dumped)" et j'aimerais savoir si quelqu'un connait la solution?

cpersonne.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
16
17
18
class CPersonne{
	private:
	string *nom, *prenom, *id, *mail;
	public:
		CPersonne();
		CPersonne(string, string, string, string);
		void set_nom(string);
		void set_prenom(string);
		void set_id(string);
		void set_mail(string);
		string get_nom() const;
		string get_prenom() const;
		string get_id() const;
		string get_mail() const;
		friend bool operator ==(CPersonne, CPersonne);
		friend ostream& operator<<(ostream& out,CPersonne p);
		~CPersonne();
};

cpersonne.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
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
56
57
58
59
60
CPersonne::CPersonne(){
	this->nom = new string;
	this->prenom = new string;
	this->id = new string;
	this->mail = new string;
}
 
CPersonne::CPersonne(string nom, string prenom, string id, string mail){
	 this->nom = new string(nom);
	 this->prenom = new string(prenom);
	 this->id = new string(id);
	 this->mail = new string(mail);
 }
 
 void CPersonne::set_nom(string nom){
	 this->nom = new string(nom);
 }
 
void CPersonne::set_prenom(string prenom){
	this->prenom = new string(prenom);
}
 
void CPersonne::set_id(string id){
	this->id = new string(id);
}
 
void CPersonne::set_mail(string mail){
	this->mail = new string(mail);
}
 
string CPersonne::get_nom() const{
	return *(this->nom);
}
 
string CPersonne::get_prenom() const{
	return *(this->prenom);
}
 
string CPersonne::get_id() const{
	return *(this->id);
}
 
string CPersonne::get_mail() const{
	return *(this->mail);
}
 
bool operator ==(CPersonne p1, CPersonne p2){
	return (p1.get_id() == p2.get_id());
}
 
ostream& operator<<(ostream& out,CPersonne p){
	return out<<p.get_nom()<<", "<<p.get_prenom()<<", "<<p.get_id()<<", "<<p.get_mail()<<"."<<endl;
}
 
CPersonne::~CPersonne(){
	delete nom;
	delete prenom;
	delete id;
	delete mail;
}

main.cpp:

Code :

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

1
2
3
4
5
6
7
8
int main(int argc, char const* argv[]){
	CPersonne p1 ("Lau", "Cédric", "1", "xxxx");
	CPersonne p2 ("Cou", "Patrick", "1", "xxx");
	if(p1 == p2){
		cout<<"C'est bon."<<endl;
	}
	cout<<p1;
}

Merci d'avance si quelqu'un a le temps et la solution à m'apporter!

[jo_link_noir]

Ne pas utiliser de pointeurs, c'est la meilleure solution.
Ne pas utiliser new est aussi une excellente solution.
Utiliser std::unique_ptr pour les ressources dynamique est généralement une des meilleurs solutions quand l'allocation "manuelle" est requise (c'est à dire pas souvent).

Ensuite, pourquoi l'erreur ? Parce qu'une copie de la classe (cf: operator<<(ostream &, CPersonne)) va copier les pointeurs, détruire l'objet local en fin de fonction (et libérer les pointeurs) puis détruire l'objet d'origine (en fin de fonction main) et re-libérer les pointeurs. Résultat: de libération, un segfault.
Toujours faire un constructeur de copie profonde (ou le supprimer) pour les classes qui utilisent des ressources allouées dynamiquement.
Utiliser std::unique_ptr pour les pointeurs permet de s'affranchir de la libération manuelle (il le fait pour nous et mieux que nous) et faire une copie aurait fait une erreur de compilation.

Note: paramètre en référence constante std::string -> std::string const &.

[dalfab]

Bonjour,

Le plantage doit certainement arriver à la dernière ligne du code.

* La règle des 3 :
Si on défini un destructeur, un constructeur par copie et/ou un opérateur de copie. Il faut aussi définir les trois.
En créant l'opérateur de copie le plantage devrait disparaître.

* Comment passer les paramètres objet de manière opportune ?
Si on avait passé les CPersonne par const CPersonne&, le plantage ne se serait pas produit.

* Bien utiliser les pointeurs et les new.
Ne pas allouer par new des objets que l'on peut créer directement, sinon on se tend des pièges. Si les allocations dynamiques n'avaient pas été utilisées, il n'y aurait pas eu plantage. Et les deux règles précédentes n'auraient pas manquées.*

grilled

[Caidriq]

Dans ma surcharge de l'opérateur << j'ai changé CPersonne par const CPersonne& et j'ai toujours l'erreur. En ce qui concerne l'utilisation des pointeurs je suis d'accord avec vous sauf que c'est un TP noté et je suis obligé de les utiliser comme inscrit dans le code. Qu'est-ce qu'un opérateur(constructeur?) de copie?

[Bousk]

C'est plus viceux et mal que ça, le crash survient sur la ligne == puisque l'opérateur utilise des copies.
https://en.wikipedia.org/wiki/Operat..._C_and_C%2B%2B

[dalfab]

En effet, le remplacement des paramètres CPersonne en CPersonne const& est à faire dans toutes les fonctions (ici déclaration et définition de operateur<< et de operateur==)
Cela devrait fonctionner.


Mais cela reste incomplet pour un avoir un code valide.


Sur la mauvaise utilisation du new, les fonctions set_nom() set_prenom() set_id() set_mail() sont incorrectes

Code :

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

1
2
3
void CPersonne::set_nom(string nom){
	 this->nom = new string(nom);
 }

Vois-tu pourquoi ? Indice : qui a-t-il dans this->nom au début de la fonction ?

Un constructeur de copie, permet de créer un objet à partir d'un objet du même type, il existe toujours implicitement, on doit en définir explicitement un, surtout s'il y a des allocations dynamiques par :

Code :

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

1
2
3
4
5
6
CPersonne::CPersonne( CPersonne const& p ) {
    nom = new string( p.nom );
    prenom = new string( p.prenom );
    id = new string( p.id );
    mail = new string( p.mail );
}

Quand on veut passer un objet en paramètre comme tu l'as fait, c'est lui qui est utilisé, c'est pourquoi le passage d'objets par const& plus optimal évite de créer cet intermédiaire.

Un opérateur de copie, permet de copier un objet dans un objet de même type, il existe toujours implicitement, il faut aussi le définir par :

Code :

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

1
2
3
4
5
6
7
CPersonne& CPersonne::operator=( CPersonne const& p ) {
    *nom = *p.nom;
    *prenom = *p.prenom;
    *id = *p.id;
    *mail = *p.mail;
    return *this;
}

Il est lui utilisé si on fait p1 = p2, et sa définition ci-dessus, permet d'éviter un plantage dans ce cas.

S'il n'y avait pas d'allocation par new, la gestion directe ou l'utilisation des unique_ptr<> n'auraient pas nécessité toutes ces subtilités. J'ai des doutes sur la bonne chronologie pédagogique de certains cours

[stendhal666]

Un opérateur de copie, permet de copier un objet dans un objet de même type, il existe toujours implicitement, il faut aussi le définir par :


CPersonne& CPersonne::operator=( CPersonne const& p ) {
*nom = *p.nom;
*prenom = *p.prenom;
*id = *p.id;
*mail = *p.mail;
return *this;
}

Il est lui utilisé si on fait p1 = p2, et sa définition ci-dessus, permet d'éviter un plantage dans ce cas.

En fait il reste une petite amélioration à faire. Les affectations (*nom = *p.nom, etc.) sont susceptibles de générer des exceptions. Si c'est le cas, les membres dont la nouvelle affectation est postérieure au lancement de l'exception ne seront pas détruits. Pour certaines ressources (fichier, mutex, grosses empreintes mémoire), ce peut être catastrophique. Il me semble (en restant dans l'optique C++98 ou antérieur du code demandé dans ce cours) que la façon de faire canonique est:

Code :

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

1
2
3
4
5
6
7
8
9
10
11
 
CPersonne& CPersonne::operator=( CPersonne const& p ) {
 
    CPersonne sink;
    std::swap(*this, sink);  // on place dans sink les membres de this. Comme sink est alloué sur la pile, il sera détruit et donc les ressources libérées, à la fin de ce bloc d'éxécution, quoiqu'il arrive
    *nom = *p.nom; // on peut continuer comme ça car le constructeur alloue toujours de la mémoire même sans argument.
    *prenom = *p.prenom;
    *id = *p.id;
    *mail = *p.mail;
    return *this;
}

Encore mieux, un constructeur sans argument qui initialise les pointeurs membres à nullptr, puis:

Code :

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

1
2
3
4
5
6
7
8
9
10
11
 
CPersonne& CPersonne::operator=( CPersonne const& p ) {
 
    CPersonne sink;
    std::swap(*this, sink);  
 
    CPersonne tmp(p); // on réutilise le constructeur par copie, pas de code redondant
    std::swap(*this, tmp);
 
    return *this;
}

[ternel]

Si swap est correctement écrite, il n'y a normalement pas besoin de swapper *this vers sink.

cppreference propose une liste de formes canoniques

La solution idiomatique est "copy-and-swap"

Code :

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

1
2
3
4
CPersonne& CPersonne::operator=( CPersonne p ) {
    swap(p);
    return *this;
}

[Caidriq]

Un constructeur de copie, permet de créer un objet à partir d'un objet du même type, il existe toujours implicitement, on doit en définir explicitement un, surtout s'il y a des allocations dynamiques par :

Code :

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

1
2
3
4
5
6
CPersonne::CPersonne( CPersonne const& p ) {
    nom = new string( p.nom );
    prenom = new string( p.prenom );
    id = new string( p.id );
    mail = new string( p.mail );
}

Quand on veut passer un objet en paramètre comme tu l'as fait, c'est lui qui est utilisé, c'est pourquoi le passage d'objets par const& plus optimal évite de créer cet intermédiaire.

J'ai bien pigé tout ce que tu me dis mais le constructeur de copie ne fonctionne pas, il me met des erreurs. :'(

[Caidriq]

Ah non c'est bon j'ai trouvé la soluce!Merci les gars

[dalfab]

@Caidriq, as-tu corrigé l'erreur des fonction set_... , le problème a pu être noyé dans les remarques qui ont suivi ?

aux autres (sauf Caidriq qui a déjà beaucoup de choses à ingurgiter)

Les 1er constructeurs devraient être repris pour être tolérant aux exceptions, par exemple :

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
CPersonne::CPersonne( string nom , string prenom , string id , string mail )
 : nom( new string( std::move( nom ) ) )
{
    this->prenom = new( std::nothrow ) string( std::move( prenom ) );
    this->id = new( std::nothrow ) string( std::move( id ) );
    this->mail = new( std::nothrow ) string( std::move( mail ) );
 
    if ( !this->prenom  ||  !this->id  ||  !this->mail )
    {  // au moins 1 n'a pas pu être initialisé, tout nettoyer et signaler l'anomalie d'allocation échouée
        delete this->mail;
        delete this->id;
        delete this->prenom;
        delete this->nom;
 
        throw std::bad_alloc;
    }
}

[Caidriq]

Non je n'ai pas corrigé, quel est le soucis dans mes set?

[dalfab]

On ne doit pas refaire un new sur un objet déjà alloué, sinon une allocation sera perdue

Code :

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

1
2
3
void CPersonne::set_nom(string nom){
	 this->nom = new string(nom);
 }

est à remplacer par :

Code :

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

1
2
3
4
void CPersonne::set_nom(string nom){
         delete this->nom;
	 this->nom = new string(nom);
 }

ou par :

Code :

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

1
2
3
void CPersonne::set_nom(string nom){
	 *this->nom = nom;
 }

[Caidriq]

Ah bein oui je suis con, sinon on alloue une autre place mémoire pour rien!

[koala01]

Salut,

La toute première chose à comprendre, c'est que les pointeurs ne doivent être utilisés que quand on n'a vraiment pas d'autre choix!.

Nous ne sommes pas en java, ici, mais en C++! et, s'il faut effectivement passer par des pointeurs et par l'utilisation de new en java (pour que tout le système fonctionne), il ne faut recourir à l'allocation dynamique de la mémoire en C++ que... quand on a effectivement affaire à une donnée dont le type réel doit être déterminé à l'exécution (et encore!).

Ainsi, tes données membres nom, prenom, id et mail n'ont absolument aucune raison d'être des pointeurs: la classe std::string s'occupe d'elle-même de gérer l'espace mémoire nécessaire à la représentation de la chaine de caractères bien mieux que ce que tu ne pourra jamais le faire à la main! Si bien que ta classe devrait en réalité prendre la forme de

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
56
57
58
59
60
61
class CPersonne{
	private:
        /* une variable par ligne, c'est beaucoup plus facile à lire et à comprendre
         *
         * la directive using namespace std; est une aberration, surtout si elle se trouve dans un fichier d'en-tête
         * il faut donc utiliser le nom pleinement qualifié des fonctionnalités proposées par la S(T)L !
         */
	std::string nom;
        std::string prenom;
        std::string id;
        std::string mail;
	public:
                /* !!!! Toute les personnes doivent au minimum être identifiées par un nom, un prénom, et un id!!!
                 * le constructeur par défaut n'a donc absolument aucun intérêt !!!!!
                 */
		//CPersonne();
                /* Dés que l'on décide de transmettre -- pour faire simple -- une structure ou une classe à une fonction,
                 * il est préférable de la transmettre par référence, pour éviter les copies inutiles
                 * Si la fonction n'a pas vocation à modifier la valeur en question, nous choisirons de la transmettre sous
                 * la forme d'une référence constante
                 * le fait d'indiquer le nom des paramètres peut avoir une influence positive sur la compréhension du code
                 * nous pouvons en outre définir une valeur par défaut (connue pour être invalide) à l'adresse E-mail
                 */
		CPersonne(std::string const & id, std::string const & nom, std::string const & prenom, std::string const & mail="invalid@invalid.com");
                /* Il n'y a aucune raison pour que l'on décide de changer le nom, le prénom ou l'id d'une
                 * personne!!!
                 * Ces informations sont fournies à la naissance / à la création, et restent les même jusqu'à
                 * la mort / la destruction de ta personne!!!
                 * il n'y a donc aucune raison de fournir les fonctions set_nom, set_prenom et set_id (*)
                 */
		// void set_nom(string);
		// void set_prenom(string);
		//void set_id(string);
                /* l'adresse mail est la seule information qui ** eventuellement ** susceptible d'évoluer dans le
                 * temps...  Et encore!!! 
                 */
		void set_mail(std::string const & newmail);
                /* autant renvoyer une référence constante sur les données membres, pour éviter (une fois encore)
                 * les copies inutiles
                 */
		std::string const & get_nom() const;
		std::string const & get_prenom() const;
		std::string const & get_id() const;
		std::string const & get_mail() const;
                /* lorsque tu transmet une personne, l'idéal est de la transmettre sous la forme d'une référence,
                 * voire, sous la forme d'une référence constante si la fonction n'a pas vocation à modifier la personne!
                 *
                 * NOTA: une classe CPersonne a -- de toutes évidences -- sémantique d'entité...  Il n'y aurait normalement
                 * aucune raison de vouloir comparer deux personnes entre elles, bien que l'on puisse envisager de comparer
                 * les différentes données auxquelles on a accès ;)
                 */
		friend bool operator ==(CPersonne const &, CPersonne const &);
		friend ostream& operator<<(ostream& out,CPersonne const & p);
                /* si l'on n'utilise plus les pointeur, le compilateur sait exactement comment faire pour détruire notre
                 * personne!!!  A moins de prévoir que l'on aura affaire à une hiérarchie de classe dont CPersonne serait
                 * la classe de base, et que l'on voudra pouvoir être en mesure de détruire n'importe quelle personne alors
                 * que nous la connaissons comme étant... de type CPersonne (ce qui justifierait un destructeur virtuel),
                 * nous pouvons donc laisser le compilateur fournir lui-même le destructeur ;)
                 */
		//~CPersonne();
};

et l'implémentation prendrait alors la forme de

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
 
CPersonne::CPersonne(std::string const & id, std::string const & nom, std::string const & prenom, std::string const & mail):
    nom(nom),prenom(prenom),id(id),mail(mail){ //utilisation de la liste d'initialisation
}
std::string const & CPersonne::get_nom() const{
    return nom;
}
std::string const & CPersonne::get_prenom() const{
    return prenom;
}
std::string const & CPersonne::get_id() const{
    return id;
}
std::string const & CPersonne::get_mail() const{
    return mail;
}
void CPersonne::setMail(std::string const & newmail){
    mail=newmail;
}
bool operator==(Cpersonne const & a, CPersonne const & b){
    //  NOTA : a priori, la valeur de id seule devrait suffire amplement
    // pour déterminer si deux personnes sont identiques!
    return a.get_nom() == b.get_nom() &&
              a.get_prenom() == b.get_prenom() &&
              a.get_id() == b.get_id();
}
std..ostream & operator<< (std::ostream & os, CPersonne const & p){
    os<<"id = "<<p.get_id()<<"\n"
        <<"nom = "<<p.get_nom()<<"\n"
        <<"prenom = "<<p.get_prenom()<<"\n"
        <<"e-mail "<<(p.get_mail()!="invalid@invalid.com"? p.get_mail() : "aucune adresse valide")<<"\n";
}

Avoues que cela facilite grandement les choses, par rapport à l'utilisation des pointeurs, non

(*) La classe CPersonne devrait en toute logique avoir sémantique d'entité, ce qui fait que ce devrait être une classe non copiable et non affectable! En outre, le nom, le prénom et l'id peuvent très vraisemblablement servir dans un processus d'identification des différentes personnes dont tu disposes dans ton application!

[Luc Hermitte]

[tiens, grillé]

Tu veux dire que ton prof t'oblige à mettre des étoiles ici? "string *nom, *prenom, *id, *mail;"
pfff.

C'est pratiquement impossible d'écrire un constructeur garanti sans fuites de mémoire avec une telle spéc idiote. C++ n'est pas Java!

Cela ressemblerait à ça:

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
struct Toto {
    Toto(std::string const& a, std::string, const& b) // quasi idem avec celui par défaut, ce qui est probablement une mauvaise idée
     : m_a(new std::string(a)) // peut fuir si le deuxième échoue, dis merci à ton prof
     , m_b(new std::string(b)) // imagine qu'une exception soit lancée ici -> pouf, la mémoire de m_a est perdue à tout jamais!
    {}
    // La copie est tout aussi simple
    Toto(Toto const& rhs) 
     : m_a(new std::string(rhs.m_a)) // peut fuir si le deuxième échoue, dis merci à ton prof
     , m_b(new std::string(rhs.m_b))
    {}
   Toto& operator=(Toto rhs) // oui, par copie
   { this->swap(rhs);  return *this; }
   void swap(Toto & o)
   {   using std::swap;
       swap(o.m_a, m_a); // échanger les pointeurs suffit ici
       swap(o.m_b, m_b);
   }
private:
   std::string * /* une étoile qui n'a rien à faire là!!! */ m_a;
   std::string * /* une étoile qui n'a rien à faire là!!! */ m_b;
};

NB: sans ces fichus pointeurs qui déstabilisent tout, tu n'aurais eu à écrire que les deux premiers constructeurs. Et le programme eut été bien plus correct. i.e. garanti sans fuite, ce qui est impossible (je simplifie) avec plus d'une ressource brute.

PS: et comme l'a dit Koala01, la copie aurait dû être interdite sur cette classe. Un meilleur exo sur ce thème est l'écriture d'une classe matrice.

[Caidriq]

Oui je sais Koala mais c'est dans le cadre d'un TP noté, donc on est forcé d'utiliser les pointeurs de cette manière. J'imagine que c'est pour nous apprendre comment fonctionnent les pointeurs et leur utilité.

[koala01]

Oui je sais Koala mais c'est dans le cadre d'un TP noté, donc on est forcé d'utiliser les pointeurs de cette manière. J'imagine que c'est pour nous apprendre comment fonctionnent les pointeurs et leur utilité.

Ben, tu pourras dire à ton prof que c'est un imbécile, qui vous oblige à faire des choses totalement absurdes et qui n'atteignent surement pas les objectifs qu'il croit poursuivre!

Il y a -- effectivement -- "quelque chose" à faire pour permettre aux étudiants de comprendre les principes liés à l'allocation dynamique de la mémoire (bien que les pointeurs intelligents apparus en C++11 nous permettent de gérer la mémoire de manière bien plus correcte sans se prendre le choux ), mais, pour qu'un exemple puisse avoir la moindre utilité, il faut qu'il puisse être considéré comme conceptuellement correct!

Il n' y a rien à faire! pour qu'un exemple soit utile, il faut que vous ayez une chance -- aussi minime soit elle -- d'être confronté à une situation similaire dans votre vie professionnelle! Si un développeur venait avec un tel code dans mon équipe, je ne sais pas si je lui apprendrais à voler en le faisant passer par la fenêtre ou si je le condamnerais à être attaché par les pieds, couvert de miel au dessus d'un nid de fourmis rouges!

Et le problème est d'autant pire que je parie que ton prof n'aura jamais insisté sur le fait que l'appel à new risque de lancer une exception et qu'il y a donc de très gros problèmes prendre en compte pour éviter les problèmes si id = new std::string(id); vient à échouer alors que nom = new std::string(nom); et prenom = new std::string(prenom); ont réussi juste avant

Ah, mais non! ton prof t'incite à vivre dans le monde des bisounours, dans lequel tout le monde il est gentil, tout le monde il est beau et une allocation dynamique de la mémoire ne peut pas échouer!!! Sauf que la dure réalité de la vie est tout autre!

[Caidriq]

Je sais pas c'est une doctorante donc il est fort probable qu'elle nous enseigne de la grosse grosse théorie pas du tout applicable dans le monde professionnel.

[mintho carmo]

pas du tout applicable dans le monde professionnel

C'est génial une école/université qui apprend des loisirs à ses étudiants...
Par contre, n'hésites pas à te former à côté pour apprendre à programmer correctement, cela pourra te servir quand tu bosseras.