clone(), c'est le mal (?)

Version imprimable

28/11/2012, 17h30
r0d

clone(), c'est le mal (?)

J'ai eu une discussion intéressante avec des collègues ce midi: pour ou contre la fonction clone(). Elle n'est certes pas d'une importance capitale, mais soulève quelques problématiques intéressantes je trouve.

Moi je suis contre car:
0. Bêtement et concrètement, je n'en ai jamais eu besoin.
1. Il vaut mieux implémenter un constructeur par copie.
2. On ne peut pas faire du NVI si on a du clone() sur des classes destinées à être héritées.
3. clone() peut être utile, justement, dans le but d'utiliser le polymorphisme. Mais dans ce cas, ne vaut-il pas mieux utiliser une abstract factory?

Qu'en pensez-vous?

Cordialement,
r0d.
28/11/2012, 17h59
Jean-Marc.Bourguet

Citation:

Envoyé par r0d

J'ai eu une discussion intéressante avec des collègues ce midi: pour ou contre la fonction clone(). Elle n'est certes pas d'une importance capitale, mais soulève quelques problématiques intéressantes je trouve.

Moi je suis contre car:
0. Bêtement et concrètement, je n'en ai jamais eu besoin.

C'est un argument qui n'emporte pas grand chose.

Citation:

1. Il vaut mieux implémenter un constructeur par copie.

C'est une affirmation. Les deux me semblent bien different a moi, ou plutot le clone() est un constructeur de copie polymorphique. Donc si tu as une semantique de valeur, tu as generalement un constructeur de copie, pas de polymorphisme et pas de clone. Si tu as une semantique d'entite, tu n'as generalement pas de constructeur de copie (ou du moins protected) et tu peux avoir un close si la copie fait quand meme du sens.

Citation:

2. On ne peut pas faire du NVI si on a du clone() sur des classes destinées à être héritées.

Explique ton raisonnement, je vais finir par croire qu'on entend des choses differentes par clone. Clone n'a du sens que sur une classe destinee a etre heritee et je ne vois pas de probleme avec le NVI.

Citation:

3. clone() peut être utile, justement, dans le but d'utiliser le polymorphisme. Mais dans ce cas, ne vaut-il pas mieux utiliser une abstract factory?

Ca me semble independant. Et je vois bien clone utilisé par un certain type d'abstract factory pour son fonctionnement.
28/11/2012, 19h20
r0d

Citation:

Envoyé par Jean-Marc.Bourguet

C'est un argument qui n'emporte pas grand chose.

C'était plus une tentative d'humour qu'un argument. Passons.

Citation:

Envoyé par Jean-Marc.Bourguet

Explique ton raisonnement, je vais finir par croire qu'on entend des choses differentes par clone. Clone n'a du sens que sur une classe destinee a etre heritee et je ne vois pas de probleme avec le NVI.

Je me trompe sans doute, et c'est la raison pour laquelle j'ai ouvert cette discussion. Mais il me semble que le clone() de la classe mère va, généralement, être virtuelle pure. On ne peut pas cloner un objet si on ne sais pas exactement ce qu'il contient; on ne peut pas faire de "clonage partiel", ou alors ce ne serait plus un clone. Et donc, fonction virtuelle pure => plus de NVI.

Citation:

Envoyé par Jean-Marc.Bourguet

Ca me semble independant. Et je vois bien clone utilisé par un certain type d'abstract factory pour son fonctionnement.

Tu as certainement raison, mais je ne parviens pas à trouver un exemple.

Après, j'ai tout de même trouvé un cas où le clone() est fort utile: http://en.wikipedia.org/wiki/Curious...y_construction. Je trouve ce pattern fort utile et je pense que je vais l'intégrer dans ma "boite à outil". Mais cela reste, selon moi, un cas particulier.
28/11/2012, 19h47
Jean-Marc.Bourguet

Citation:

Envoyé par r0d

C'était plus une tentative d'humour qu'un argument. Passons.

Ma reponse etait sur le meme ton; notre humour a l'air d'etre aussi convainquant que tes arguments ;)

Citation:

Je me trompe sans doute, et c'est la raison pour laquelle j'ai ouvert cette discussion. Mais il me semble que le clone() de la classe mère va, généralement, être virtuelle pure. On ne peut pas cloner un objet si on ne sais pas exactement ce qu'il contient; on ne peut pas faire de "clonage partiel", ou alors ce ne serait plus un clone. Et donc, fonction virtuelle pure => plus de NVI.

C'est quoi NVI pour toi? (le pattern habituel Base* clone() { return do_clone(); } me semble applicable).

Citation:

Tu as certainement raison, mais je ne parviens pas à trouver un exemple.

Dans une approche pour fonctionner comme un systeme transactionnel, on peut faire un clone de ce sur quoi on va travailler et a la fin ou committer le clone apres modification (et delete de l'original) ou conserver l'original (et delete du clone). C'est un point de compromis different qu'un systeme base sur des vues.
28/11/2012, 19h49
r0d

Citation:

Envoyé par Jean-Marc.Bourguet

C'est quoi NVI pour toi? (le pattern habituel Base* clone() { return do_clone(); } me semble applicable).

Ok. En fait j'avais mal compris le pattern clone(). Comme quoi, j'ai bien fait d'ouvrir cette discussion :)
Je retourne à mes livres :aie:
Merci pour ton intervention.

Salut,

Ce qui est vrai, c'est que tu dois choisir entre le retour co variant (qui te permettrait de récupérer un pointeur sur un objet de type dérivé) et le NVI, mais, de manière générale, tu es de toutes manières obligé de choisir entre les deux ;)

De plus, si tu considères que les classes ayant sémantique d'entité sont non copiables (car elles devraient l'être), c'est peut etre l'une des seules possibilité qui te sera donnée si tu dois manipuler des pointeurs intelligents (unique_ptr en premier) dans un pattern composite.

Imaginons que, dans ton pattern composite, tu doive pouvoir gérer un grand nombre de types distincts (par exemple, un widget de n'importe quel type concret qui peut avoir comme enfant un nombre quelconque de widget de n'importe quel type concret).

Pour que les enfants soient détruits automatiquement, et dans une optique RAII, tu utiliseras sans doute un (conteneur de) std::unique_ptr pour contenir les enfants, ce qui correspondrait à quelque chose comme
Code:

1 2 3 4 5 class Widget { private: std::vector<std::unique_ptr<Widget>> children_; };
Tu pourrait, bien sur, ajouter une fonction addChild qui prendrait un pointeur nu sur Widget et laisser la fonction s'occuper de gérer l'insertion du unique_ptr, mais, à mon sens, cela aurait deux défauts majeurs:

Le premier serait peut etre de laisser croire à l'utilisateur qu'il "garde le controle" de la durée de vie de l'objet pointé, car sa logique pourrait se baser sur un
Code:

1 2 3 4 5 6 7 Widget * ptr = new DerivedWidget(/* paramètres */); /* holder est de type widget ;) */ holder.addWidget(ptr); /*...*/ //Mince, il ne faut pas que j'oublie de détruire mon pointeur delete ptr;
sauf que, dans le cas présent, ce n'est plus à l'utilisateur de s'occuper de la libération de la mémoire, vu qu'elle sera prise en charge par le unique_ptr :aie:

Le deuxième défaut est le phénomène inverse : L'utilisateur pourrait, à tord, penser que la fonction addWidget crée de toutes manières un clone / une copie du pointeur passé, car il garde en tete qu'il sera géré par un unique_ptr et écrire un code proche de
Code:

1 2 DerivedWidget temp(/*...*/ ); holder.addChild(&temp);
avec les conséquences désastreuse que cela ne manquera pas d'entrainer, car le pointeur sera invalidé dés la sortie de la portée et que, si on arrive jusque là, appeler delete sur une adresse mémoire qui n'a pas été allouée dynamiquement occasionne des catastrophes :aie:

Si, par contre, la fonction addChild prend une référence constante sur un widget (et plus encore si elle renvoie une référence non constante sur l'enfant effectivement inséré), tu retires de l'esprit de l'utilisateur tout doute quant au fait qu'il doit ou non utiliser l'allocation dynamique:

Personnellement, si je suis en présence d'une fonction prenant une référence constante sur un objet, ou bien j'utilise une des variables que j'ai à ma disposition, ou bien j'utilise une variable temporaire anonyme.

Mais, en tout état de cause, je ne vais pas créer spécialement un objet par allocation dynamique pour transmettre "ce qui est pointé par le pointeur" que je viens de déclarer à la fonction ;)

Le problème est, bien sur, que pour passer d'une référence constante à un pointeur sur objet alloué dynamiquement nécessaire à unique_ptr, ben, il faut pouvoir copier le dit objet :aie:

Et comme l'objet est, par défaut non copiable, il ne reste plus qu'une solution : le clonage ;)

Au final, je verrais bien ma classe Widget sous une forme proche de
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 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 class Widget { public: /* je ne m'intéresse ici qu'à l'interface de gestion des enfants et au clonage ;) */ Widget & addChild(Widget const & toadd) { children_.push_back(std::move(std::unique_ptr<Widget>(toadd.clone())); return child(childCount()-1); } size_t childCount() const { return children_.size(); } Widget & child(size_t index) { return const_cast<Widget&>( static_cast<const Widget*>(this)->child(index)); } Widget const & child(size_t index) const { if(index >= childCount()) throw IndexOutOfBoundException(); return *(children_[index].get()); } void removeChild(size_t index) { if(index >= childCount()) throw IndexOutOfBoundException(); children_.erase (children_.begin()+index); } Widget * clone() const; { Widget *temp = doClone(); for(size_t i = 0; i<childCount();++i) temp->addWidget(child(i)); return temp; } private: std::vector<std::unique_ptr<Widget>> children_; virtual Widet * doClone() const = 0; };

29/11/2012, 09h28
r0d

Intéressant :ccool:
Quelle est la différence entre:

Code:

children_.push_back(std::move(std::unique_ptr<Widget>(toadd.clone()));

et

Code:

children_.push_back(std::unique_ptr<Widget>(toadd.clone());

:question:
29/11/2012, 09h35
koala01

Un unique_ptr ne peut pas etre copié, et je ne suis pas sur que push_back implémente la sémantique de mouvement :aie:

Peut etre que le code sur lequel je me suis basé est simplement trop vieux, peut etre s'agit il d'un "bug" de gcc car la sémantique de mouvement devrait etre appliquée sur push_back :question: les deux solutions sont possibles ;)

Une chose est sure, c'est que si tu utilises explicitement la sémantique de mouvement à bon escient, tu est certain qu'elle sera appliquée même si son support n'est encore que partiel (comprend par là : meme si toutes les fonctions de toutes les collection qui devraient l'implémenter ne le font pas encore ;))
29/11/2012, 09h40
Jean-Marc.Bourguet

Ou j'ai manque quelque chose -- ce qui n'est pas impossible, mes cibles habituelles n'ont pas du tout de support de C++11 -- ou utiliser std::move sur autre chose que des variables n'a pas d'interet, son objectif etant de transformer quelque chose qui n'est pas un temporaire en un temporaire.
29/11/2012, 09h43
r0d

Au fait, le unique_ptr, ça fait longtemps qu'il est dans la STL? C'était pas un smart pointer de boost avant?
29/11/2012, 09h59
gbdivers

@koala01
push_back prend bien en charge le move (gcc 4.8, avant j'ai pas vérifié)

@Jean-Marc.Bourguet
plus précisément, ça transforme une lvalue en rvalue (et ne fait donc rien sur une rvalue, mais c'est pas interdit)
J'ai un article en cours de préparation là dessus, mais c'est un peu long

@r0d
Depuis le C++11 (peut être même dans le TR1 ?). Et oui, c'était dans boost avant. Et c'est dans gcc depuis un bout de temps
29/11/2012, 10h08
ternel

Une partie de boost a été importé dans la STL avec la C++11.

C'est le cas des smarts pointers, du foreach (for(type v: collection)), et d'autres choses.
29/11/2012, 14h13
JolyLoic

Mais pas de unique_ptr, qui a besoin de la sémantique de déplacement pour être utile, alors que boost se basais à l'époque sur des compilateurs existants qui ne l'avaient pas.

Citation:

Envoyé par gbdivers

@koala01
push_back prend bien en charge le move (gcc 4.8, avant j'ai pas vérifié)

Ah, ils ont corrigé l'erreur alors, car en Gcc 4.7.1, le code

Code:

1
2
3
4
5
6
int main(int argc, char** argv)
{
    std::vector<std::unique_ptr<int>> tab;
    tab.push_back(std::unique_ptr<int>(new int));
    return 0;
}

fonctionne alors que le code

Code:

1
2
3
4
5
6
7
int main(int argc, char** argv)
{
    std::vector<std::unique_ptr<int>> tab;
    std::unique_ptr<int> temp(new int);
    tab.push_back(temp);
    return 0;
}

échoue à la compilation avec les messages

Code:

1
2
3
4
5
6
7
8
9
 
c:\mingw\bin\..\lib\gcc\x86_64-w64-mingw32\4.7.1\include\c++\ext\new_allocator.h||In instantiation of 'void __gnu_cxx::new_allocator<_Tp>::construct(_Up*, _Args&& ...) [with _Up = std::unique_ptr<int>; _Args = {const std::unique_ptr<int, std::default_delete<int> >&}; _Tp = std::unique_ptr<int>]':|
c:\mingw\bin\..\lib\gcc\x86_64-w64-mingw32\4.7.1\include\c++\bits\alloc_traits.h|250|required from 'static typename std::enable_if<std::allocator_traits<_Alloc>::__construct_helper<_Tp, _Args>::value, void>::type std::allocator_traits<_Alloc>::_S_construct(_Alloc&, _Tp*, _Args&& ...) [with _Tp = std::unique_ptr<int>; _Args = {const std::unique_ptr<int, std::default_delete<int> >&}; _Alloc = std::allocator<std::unique_ptr<int> >; typename std::enable_if<std::allocator_traits<_Alloc>::__construct_helper<_Tp, _Args>::value, void>::type = void]'|
c:\mingw\bin\..\lib\gcc\x86_64-w64-mingw32\4.7.1\include\c++\bits\alloc_traits.h|387|required from 'static void std::allocator_traits<_Alloc>::construct(_Alloc&, _Tp*, _Args&& ...) [with _Tp = std::unique_ptr<int>; _Args = {const std::unique_ptr<int, std::default_delete<int> >&}; _Alloc = std::allocator<std::unique_ptr<int> >]'|
c:\mingw\bin\..\lib\gcc\x86_64-w64-mingw32\4.7.1\include\c++\bits\stl_vector.h|885|required from 'void std::vector<_Tp, _Alloc>::push_back(const value_type&) [with _Tp = std::unique_ptr<int>; _Alloc = std::allocator<std::unique_ptr<int> >; std::vector<_Tp, _Alloc>::value_type = std::unique_ptr<int>]'|
D:\projects\forumcpp\main.cpp|27|required from here|
c:\mingw\bin\..\lib\gcc\x86_64-w64-mingw32\4.7.1\include\c++\ext\new_allocator.h|110|error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = int; _Dp = std::default_delete<int>; std::unique_ptr<_Tp, _Dp> = std::unique_ptr<int>]'|
c:\mingw\bin\..\lib\gcc\x86_64-w64-mingw32\4.7.1\include\c++\bits\unique_ptr.h|256|error: declared here|
||=== Build finished: 2 errors, 5 warnings (0 minutes, 1 seconds) ===|

Mais peut etre est-ce surtout du à une utilisation impropre de unique_ptr :P

Mais, si c'est du à une mauvaise utilisation de unique_ptr (ce que je n'exclus pas, vu que je commence à peine à réellement m'intéresser à tout cela ;)), le clonage devient indispensable, entre autres si l'on souhaite pouvoir sérialiser ma classe Widget d'exemple qui a occasionné la digression concernant la sémantique de mouvement ;)

Ceci dit, toujours en Gcc 4.7.1, le code

Code:

1
2
3
4
5
6
7
int main(int argc, char** argv)
{
    std::vector<std::unique_ptr<int>> tab;
    std::unique_ptr<int> temp(new int);
    tab.push_back(std::move(temp));
    return 0;
}

compile parfaitement ;)

29/11/2012, 19h20
Jean-Marc.Bourguet

Ca me semble normal que move soit necessaire avec une variable.
29/11/2012, 19h27
gbdivers

Je crois pas que c'est une erreur

Code:

tab.push_back(temp);

temp est une lvalue donc appel de la version push-back(&)
Pour appeler la version avec rvalue ref, il faut passer une rvalue, soit en passant un temporaire :

Code:

tab.push_back(std::unique_ptr<int>(new int));

soit en convertissant une lvalue en rvalue avec move :

Code:

tab.push_back(std::move(temp));

dis autrement, comme temp est invalidé après cette opération, il faut explicitement dire que l'on veut bien un move et pas une copie (un move implicite voudrait dire que l'on pourrait se retrouver avec une variable "invalide" sans s'en rendre compte)
29/11/2012, 19h28
koala01

Citation:

Envoyé par Jean-Marc.Bourguet

Ca me semble normal que move soit necessaire avec une variable.

Citation:

Envoyé par gbdivers

Je crois pas que c'est une erreur

Code:

tab.push_back(temp);

temp est une lvalue donc appel de la version push-back(&)
Pour appeler la version avec rvalue ref, il faut passer une rvalue, soit en passant un temporaire :

Code:

tab.push_back(std::unique_ptr<int>(new int));

soit en convertissant une lvalue en rvalue avec move :

Code:

tab.push_back(std::move(temp));

dis autrement, comme temp est invalidé après cette opération, il faut explicitement dire que l'on veut bien un move

Oui, c'est effectivement le raisonnement que j'étais occupé à suivre dans ma petite tête :P ;)