Surcharge d'opérateurs et affectation

Bonsoir,

J'ai crée une classe t_matrix dont certains opérateurs ont été surchargés, afin notamment de supporter l'addition, la soustration et la multiplication de deux objets t_matrix. Les données sont stockées dans un tableau float *. Voici les prototypes de ces surcharges :

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24

// operator=: // defines operator = t_matrix & operator=(const t_matrix & toCopy);   // operator+: // computes this+arg t_matrix operator+(const t_matrix & arg);   // operator-: // computes this-arg t_matrix operator-(const t_matrix & arg);   // operator*: // computes this*arg t_matrix operator*(const t_matrix & arg);   // operator*: // computes this*arg t_matrix operator*(const double arg);   // operator*: // computes arg*_matrix friend t_matrix operator*(const double arg, const t_matrix & _matrix);

---

J'ai également défini un constructeur de copie :

Code:

---

1 2 3 4

// t_matrix: // constructor of copy t_matrix(const t_matrix & toCopy);

---

Prenons un exemple :

Code:

---

1 2 3 4 5 6 7

t_matrix A(3, 3, 1.0); // dim : m*n, valeur initiale = 1.0 t_matrix x(3, 3, 3.0); t_matrix K = A*x; t_matrix L = A; t_matrix M(3, 3, 0.0); M = A;

---

Lorsque j'effectue l'affectation via t_matrix L = A, le constructeur de copie est bien appelé et fait son travail. Lorsque j'effectue l'affection via M = A, il s'agit de l'opérateur d'affectation. Mais lorsque j'effectue A*x, qui retourne un objet de type t_matrix, ni le constructeur de copie ni l'opérateur d'affectation n'est appelé.

J'obtiens donc par la suite des erreurs de double free or corruption probablement à cause d'un double appel delete mat, où mat est de type float *.

Une subtilité (ou pas) doit probablement m'échapper... Help! :oops:

En vous remerciant d'avance,

Nicolas.

Après recompilation, le tout semble fonctionner. :x

Le constructeur de copie est bien appelé pour t_matrix L = A*x.

EDIT:

Le fait d'utiliser t_matrix & t_matrix(const t_matrix & toCopy) ou t_matrix t_matrix(const t_matrix & toCopy) semble affecter le fonctionnement du problème : l'opérateur d'affectation est appelé dans tous les cas lorsque le retour s'effectue par référence alors que le constructeur de copie et l'opérateur d'affectation sont appelés tous les deux (reste à savoir dans quels cas) lorsqu'il n'y a pas de &.

Pourquoi ? :?

Code:

---

1 2	Le fait d'utiliser t_matrix & t_matrix(const t_matrix & toCopy) ou t_matrix t_matrix(const t_matrix & toCopy)

---

Je suppose qu'il s'agit d'une maladresse mais un contructeur ne retourne rien.

Quand tu fais

Code:

---

1 2	t_matrix L = A*x

---

Effectivement, la matrice L est déclarée et initialisée pour la première fois, c'est donc bien le constructeur par copie qui est appelé :

Code:

---

1 2	t_matrix(const t_matrix& toCopy)

---

Il se charge de construire et initialiser ta matrice L à partir d'un référence sur la matrice 'TEMP' retournée par A*x.

Quel est ton problème, qu'est ce qui ne fonctionne pas?

Bonjour,

Citation:

---

Je suppose qu'il s'agit d'une maladresse mais un contructeur ne retourne rien.

---

Effectivement. Heureusement, elle ne figure pas dans mon code. :P

Selon que j'utilise cette définition :

Code:

---

1 2	t_matrix operator=(const t_matrix & arg)

---

ou celle-ci :

Code:

---

1 2	t_matrix & operator=(const t_matrix & arg)

---

pour l'opérateur d'affectation, le comportement du programme n'est pas le même. Lorsque la première définition est utilisée, l'opérateur d'affectation et le constructeur de copie sont tous les deux utilisés durant le programme (il s'agit d'un algorithme de point intérieur). Lorsque la seconde définition est utilisée, seul l'opérateur d'affectation est utilisé. Or, le résultat retourné par le programme est identique (et correct) dans les deux cas.

Je cherche donc à comprendre pourquoi... :oops:

Bien à vous,

Nicolas.

l'operator= renvoie *par définition* une reference vers l'objet modifié.
Si tu renvois une valeur, tu va la copier a chaque sortie de =.

Salut,

Citation:

---

Envoyé par Nadd

Une subtilité (ou pas) doit probablement m'échapper... Help! :oops:

---

Une erreur dans le constructeur de copie, l'opérateur d'affectation ou l'opérateur surchargé. En l'absence de leur code, difficile de répondre.

Ensuite, tu peux séparer la gestion du stockage de la classe matrice (SRP quand tu nous tiens). Cela clarifiera les différentes classes ne leur laissant qu'une seule responsabilité bien identifiée.

Citation:

---

Envoyé par 3DArchi

Ensuite, tu peux séparer la gestion du stockage de la classe matrice (SRP quand tu nous tiens). Cela clarifiera les différentes classes ne leur laissant qu'une seule responsabilité bien identifiée.

---

C'est ce que j'ai fait avec la mienne, mais du coup, je me demande quelle est la responsabilité de ma classe Matrix, complètement dépouillée... Il n'y reste que des opérateurs, en gros.
D'ailleurs, un commentaire sur le site d'Emmanuel Deloget fait mention d'un pattern "anémique". :-)

Citation:

---

Envoyé par oodini

C'est ce que j'ai fait avec la mienne, mais du coup, je me demande quelle est la responsabilité de ma classe Matrix, complètement dépouillée... Il n'y reste que des opérateurs, en gros.
D'ailleurs, un commentaire sur le site d'Emmanuel Deloget fait mention d'un pattern "anémique". :-)

---

Responsabilité d'une classe matrice ? Offrir la sémantique nécessaire pour représenter le concept mathématique d'une matrice (et rien d'autre) (*). C'est la première idée qui me vient et me semble suffisament fine (**), on remarque d'ailleurs qu'elle n'inclut aucune idée sur le stockage (par exemple).

(*) Par extension ca s'applique à n'importe quel élément des mathématiques qu'on voudrait introduire dans un architecture logiciel.

(**) Elle peut demander d'être préciser. Le concept mathématique de matrice est lié à suffisament d'autre concepts pour (au moins) se poser des questions (application linéaire sur un espace vectoriel par exemple). Ca peut aussi dépendre de ce qu'on va en faire et de la granularité qu'on désire.

Bonsoir,

Voici le code du constructeur de copie :

Code:

---

1 2 3 4 5 6

t_matrix::t_matrix(const t_matrix & toCopy) : m(toCopy.m), n(toCopy.n) { mat = new double[m*n]; cblas_dcopy(m*n, toCopy.mat, 1, mat, 1); }

---

et celui de l'opérateur d'affectation :

Code:

---

1 2 3 4 5 6 7 8 9 10

t_matrix & t_matrix::operator=(const t_matrix & toCopy) { m = toCopy.m, n = toCopy.n;   mat = new double[m*n]; cblas_dcopy(m*n, toCopy.mat, 1, mat, 1);   return *this; }

---

Sous cette version, l'opérateur d'affection est le seul a être appelé, même pour ce genre d'instruction : t_matrix y = A*x.

Les données de la matrice sont stockées dans une variable double * afin de s'aligner sur les spécifications de CBLAS.

N'ayez crainte, l'absence de vérifications sur les paramètres est volontaire. :P

Le principe de responsabilité unique est intéressant. Néanmoins, je ne pense pas en avoir l'utilité pour le moment.

Bien à vous,

Nicolas.

Salut,

Citation:

---

Envoyé par Nadd

Le principe de responsabilité unique est intéressant. Néanmoins, je ne pense pas en avoir l'utilité pour le moment.

---

Très drôle ! Un principe de base de l'architecture logicielle ? Ne pas en avoir besoin. Cela revient à dire : "je pourrais bien coder, mais je n'en ai pas besoin. Alors je pense que je vais faire que du code spaghetti." Sérieusement, c'est très drôle. Évite de dire ça à ton client quand même.

L'opérateur = provoque une fuite mémoire.

Cf l'idiom copy&swap

Pourquoi ne pas utiliser un std::vector<double> ?

A quoi ressemblent :
=> le constructeur par défaut,
=> le destructeur,
=> l'opérateur *
?

Citation:

---

Très drôle ! Un principe de base de l'architecture logicielle ? Ne pas en avoir besoin. Cela revient à dire : "je pourrais bien coder, mais je n'en ai pas besoin. Alors je pense que je vais faire que du code spaghetti." Sérieusement, c'est très drôle. Évite de dire ça à ton client quand même.

---

D'accord, d'accord ! Je retire ce que j'ai dit. Mon idée était simplement de proposer une classe matrix compacte. Je comprends bien le principe du SRP, mais la forte dépendance de ma classe vis-à-vis de CBLAS m'a poussé à ne pas me poser ce genre de questions.

Citation:

---

L'opérateur = provoque une fuite mémoire.

---

Oui. Je m'en suis rendu compte en postant mon précédent message. Ce sera corrigé. :oops:

Citation:

---

Pourquoi ne pas utiliser un std::vector<double> ?

---

CBLAS ne travaille qu'avec du double *, et puisque mon projet ne consiste qu'à fournir une couche d'abstraction à CBLAS en permettant d'écrire un code plus proche du langage mathématique, je ne pense pas avoir à recourir à l'usage de la classe std::vector. Toutefois, je suis ouvert à toute proposition. :P

Il n'y a pas de constructeur par défaut. Voici les deux constructeurs utilisés :

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12 13 14

t_matrix::t_matrix(const MKL_INT size_m, const MKL_INT size_n, const double init_v) : m(size_m), n(size_n) { mat = new double[m*n]; for(MKL_INT i = 0; i < m; i++) for(MKL_INT j = 0; j < n; j++) mat[i*n+j] = init_v; }   t_matrix::t_matrix(const MKL_INT size_m, const MKL_INT size_n, const double * m_array) : m(size_m), n(size_n) { mat = new double[m*n]; cblas_dcopy(m*n, m_array, 1, mat, 1); }

---

Le destructeur :

Code:

---

1 2 3 4 5

t_matrix::~t_matrix() { delete mat; }

---

L'opérateur * :

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12 13

t_matrix t_matrix::operator*(const t_matrix & arg) { double * _mat = new double[m*arg.n];   cblas_dgemm(CblasRowMajor, CblasNoTrans, CblasNoTrans, m, arg.n, n, 1.0, mat, n, arg.mat, arg.n, 0.0, _mat, arg.n);   t_matrix r(m, arg.n, _mat);   delete _mat;   return r; }

---

Je débute dans l'usage du C++, avec une certaine maladresse ! Faut-il définir l'opérateur * comme ami ? La matrice résultante n'est pas retournée par référence et donc copiée, n'est-ce pas ? J'imagine qu'il puisse s'agir d'une opération coûteuse, d'autant plus qu'il s'agit d'un opérateur fréquemment utilisé.

Si la matrice est retournée par référence, suis-je obligé de travailler avec des pointeurs ?

En vous remerciant d'avance, pour vos réponses et votre indulgence,

Nicolas.

@Nadd
a- ton destructeur est faux
b- je ne vois pas l'intérêt de cblas_dcopy (à moins qu'il sache faire des trucs intelligents en fonction de la cible de l'exécution) alors que l'on a memcpy
c- ton opérateur de multiplication s'y prend très mal à faire deux allocations.
Une seule doit être réalisée.
d- &v[0] est un pointeur, donc utilisable avec toute API à la C.
e- si ce n'est pas pour un exo (personnel ou pas), je te conseille vivement d'utiliser une des multiples classes matricielles qui existent déjà (eigen, boost.ublas (interfaçable avec ATLAS, non ?), ...)

@3DArchi
plus besoin de copy et affectation avec un vecteur pour membre (mais tu le sais déjà, hein ? ;))

Salut,

Citation:

---

Envoyé par Nadd

D'accord, d'accord ! Je retire ce que j'ai dit. Mon idée était simplement de proposer une classe matrix compacte. Je comprends bien le principe du SRP, mais la forte dépendance de ma classe vis-à-vis de CBLAS m'a poussé à ne pas me poser ce genre de questions.

---

Tout dépend de quelle compacité on parle : la classe ou les fichiers ? Séparer les différentes responsabilités dans différentes classes permet de (ne) se pose (que) les bonnes questions à chacune des classes et par là favorise ... un code plus compact et plus facile à lire.

Il est vrai qu'on croise beaucoup de développeurs (y compris expérimentés) qui répugnent à utiliser beaucoup de fichier et se retrouvent avec peu de fichier mais très (très très) gros. A mon sens, c'est une erreur puisque on perd grandement en lisibilité mais surtout on rend fortement dépendant des choses qui n'ont pas à l'être. Mon ressenti est que çà favorise plutôt les bugs.

Citation:

---

Envoyé par Nadd

CBLAS ne travaille qu'avec du double *, et puisque mon projet ne consiste qu'à fournir une couche d'abstraction à CBLAS en permettant d'écrire un code plus proche du langage mathématique, je ne pense pas avoir à recourir à l'usage de la classe std::vector. Toutefois, je suis ouvert à toute proposition. :P

---

Je ne connais pas CBLAS mais en général, l'un n'empêche pas l'autre. Tu peux retrouver un pointeur brut à partir d'un vecteur simplement en prenant l'adresse d'un élément. std::vector t'assure la contigüité des éléments. En fait, std::vector devrait te soulager fortement de la gestion de la mémoire.
Par exemple, pour tes deux constructeurs :

Code:

---

1 2 3 4 5 6 7 8 9 10

class t_matrix { t_matrix(const MKL_INT size_m, const MKL_INT size_n, const double init_v); t_matrix(const MKL_INT size_m, const MKL_INT size_n, const double * m_array);   private: MKL_INT m; MKL_INT n; std::vector<double> mat; };

---

Code:

---

1 2 3 4 5 6

t_matrix::t_matrix(const MKL_INT size_m, const MKL_INT size_n, const double init_v) : m(size_m), n(size_n),mat(size_m*size_n,init_v) { }   t_matrix::t_matrix(const MKL_INT size_m, const MKL_INT size_n, const double * m_array) : m(size_m), n(size_n),mat(m_array, m_array + size_m*size_n) {}

---

Mais comme j'imagine ce qui t'intéresse c'est d'utiliser cblas que tu supposes optimisée pour ce genre de chose, le second constructeur peut s'écrire :

Code:

---

1 2 3 4

t_matrix::t_matrix(const MKL_INT size_m, const MKL_INT size_n, const double * m_array) : m(size_m), n(size_n),mat(size_m*size_n) { cblas_dcopy(m*n, m_array, 1, &mat[0], 1);// &mat[0] permet d'avoir le pointeur sur le buffer brut de double }

---

Et ton constructeur par copie devient simplement :

Code:

---

1 2 3 4

t_matrix::t_matrix(const t_matrix & toCopy) : m(toCopy.m), n(toCopy.n),mat(toCopy.m*toCopy.n) { cblas_dcopy(m*n, &toCopy.mat[0], 1, &mat[0], 1); }

---

Enfin, l'utilisation du copy&swap doit simplifier l'écriture de l'opérateur d'affectation :

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12

t_matrix & t_matrix::operator=(t_matrix toCopy) { toCopy.swap(*this); return *this; }   void t_matrix::swap(t_matrix &rhs) throw () { std::swap(m,rhs.m); std::swap(n,rhs.n); std::swap(mat,rhs.mat); }

---

Note qu'en C++11, tu dois te poser la question du move constructeur .. qui à l'aide de vector devient trivial :

Code:

---

1 2 3 4

class t_matrix { public: t_matrix(t_matrix && toMove) = default;

---

et plus besoin de destructeur puisque c'est std::vector qui en a la responsabilité.

Citation:

---

Envoyé par Nadd

L'opérateur * :

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12 13

t_matrix t_matrix::operator*(const t_matrix & arg) { double * _mat = new double[m*arg.n];   cblas_dgemm(CblasRowMajor, CblasNoTrans, CblasNoTrans, m, arg.n, n, 1.0, mat, n, arg.mat, arg.n, 0.0, _mat, arg.n);   t_matrix r(m, arg.n, _mat);   delete _mat;   return r; }

---

---

L'opérateur reste fragile (que se passe-t-il pour _mat si le constructeur lève une exception) et n'est pas optimal (comme le souligne Luc).

Citation:

---

Envoyé par Nadd

Je débute dans l'usage du C++, avec une certaine maladresse ! Faut-il définir l'opérateur * comme ami ?

---

Oui.

Citation:

---

Envoyé par Nadd

La matrice résultante n'est pas retournée par référence et donc copiée, n'est-ce pas ? J'imagine qu'il puisse s'agir d'une opération coûteuse, d'autant plus qu'il s'agit d'un opérateur fréquemment utilisé.

---

Oui. Mais il y a des bibliothèques optimisées pour ça ;)

Citation:

---

Envoyé par Nadd

Si la matrice est retournée par référence, suis-je obligé de travailler avec des pointeurs ?

---

Mauvaise idée.

@Luc : je ne sais pas comment ma réponse s'est précédemment retrouvée sur le fil alors que j'étais en cours de rédaction... donc ce que tu as vu était une réponse en construction et avec des bêtises :oops:.
Ceci dit, le copy&swap redevient nécessaire s'il utilise cblas_dcopy dans le constructeur par copie (en supposant qu'il soit plus performant).

@Nadd : je vois du code fragile dans ce que tu nous présentes mais pour l'instant je ne vois pas ce qui a pu provoquer ton plantage. Il semble encore manquer d'éléments. Mais, je te conseille de commencer par prendre en compte les remarques ci-dessus (voir le conseil de Luc sur l'utilisation d'une bibliothèque existante).

Bonsoir, et merci beaucoup pour tous vos commentaires. :)

J'ai apporté les modifications suivantes :

Citation:

---

* utilisation de la classe std::vector
* mise à jour des constructeurs (comme précisé)
* suppression du destructeur
* suppression des doubles allocations
* mise à jour de l'opérateur d'affectation (comme précisé)
* passage des opérateurs + - * en ami

---

Effectivement; c'est beaucoup mieux ainsi. :)

Citation:

---

Envoyé par Luc Hermitte

si ce n'est pas pour un exo (personnel ou pas), je te conseille vivement d'utiliser une des multiples classes matricielles qui existent déjà (eigen, boost.ublas (interfaçable avec ATLAS, non ?), ...)

---

J'ai travaillé (un peu) avec boost auparavant. Malheureusement, les performances offertes étaient loin d'être suffisantes. CBLAS permet justement d'éviter ce problème. Ceci dit, il s'agit bien d'un projet personnel.

Je suis malheureusement confronté à une nouvelle difficulté. Dans la version précédente de ma classe, j'avais défini l'opérateur () de manière à ce qu'il retourne une référence vers un élément du tableau de sorte que l'on puisse faire A(i,j) = k. J'ai essayé de parvenir à un tel résultat avec la classe std::vector, mais pour y arriver je suis obligé d'écrire :

Code:

---

1 2 3 4 5 6

double & t_matrix::operator()(MKL_INT i, MKL_INT j) { // ajouter vérifications sur i et j return mat[i*n+j]; }

---

Pour t_matrix A, je ne peux plus utiliser A(i,j) lorsque l'objet est passé par référence constante à une fonction. Autrement dit, une fonction comme celle-ci :

Code:

---

1 2 3 4 5

double fun(MKL_INT i, MKL_INT j, const t_matrix & arg) { return arg(i,j)*arg(i,j); }

---

ne passe pas à la compilation. Suis-je forcé de retirer le qualificatif const, où existe-t-il une autre solution ? J'avais pensé à passer la matrice arg par copie, mais ça reste coûteux.

En vous remerciant une fois encore,

Nicolas.

Salut,
Il suffit de surcharger :

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12

double const & t_matrix::operator()(MKL_INT i, MKL_INT j) const { // ajouter vérifications sur i et j return mat[i*n+j]; }   double & t_matrix::operator()(MKL_INT i, MKL_INT j) { // ajouter vérifications sur i et j return mat[i*n+j]; }

---

Parenthèse, vu qu'il s'agit d'un projet perso -- très intéressant au demeurant (i.e. je t'encourage à le poursuivre)
Boost n'a peut-être pas les meilleures perfs pour le calcul même, mais il réalise des optimisations au niveau des simplifications des expressions. Ce qui abouti à moins d'allocations ou de copies. Là, l'exemple de l'allocation inutile dans op*() sabote les perfs que tu pourrais gagner avec CBLAS ou ATLAS.
Ceci dit, il me semblait qu'il y avait un moyen de déléguer certains calculs à des libs externes depuis boost.ublas.
Et aujourd'hui, eigen semble en passe de devenir la nouvelle référence.

Bonjour,

Citation:

---

Envoyé par 3DArchi

Il suffit de surcharger :

Code:

---

1 2 3 4 5 6 7 8 9 10 11 12

double const & t_matrix::operator()(MKL_INT i, MKL_INT j) const { // ajouter vérifications sur i et j return mat[i*n+j]; }   double & t_matrix::operator()(MKL_INT i, MKL_INT j) { // ajouter vérifications sur i et j return mat[i*n+j]; }

---

---

Effectivement... Merci ! :)

Citation:

---

Envoyé par Luc Hermitte

Parenthèse, vu qu'il s'agit d'un projet perso -- très intéressant au demeurant (i.e. je t'encourage à le poursuivre)
Boost n'a peut-être pas les meilleures perfs pour le calcul même, mais il réalise des optimisations au niveau des simplifications des expressions. Ce qui abouti à moins d'allocations ou de copies. Là, l'exemple de l'allocation inutile dans op*() sabote les perfs que tu pourrais gagner avec CBLAS ou ATLAS.
Ceci dit, il me semblait qu'il y avait un moyen de déléguer certains calculs à des libs externes depuis boost.ublas.
Et aujourd'hui, eigen semble en passe de devenir la nouvelle référence.

---

Je souhaitais bénéficier d'une simplification des expressions, en permettant d'écrire du code plus proche des mathématiques, tout en bénéficiant des performances de CBLAS. Pour être honnête, je n'ai pas cherché à creuser davantage (à tort sûrement) du côté de boost.ublas. Ceci dit, développer une couche d'abstraction minimale pour CBLAS fut (et continue d'être) un bon exercice ! :)

Le code étant à présent fonctionnel, je vous adresse mes plus sincères remerciements. :)

Bien à vous,

Nicolas.