shared_ptr et composite

Version imprimable

Bonjour à tous,

J'ai une classe conteneur directory et une classe directory_item (dont va dériver la classe directory et une classe file). Un objet directory contient une liste de directory_item (via des shared_ptr), et directory_item connaît son répertoire parent (via un weak_ptr).
La classe directory comporte une fonction membre add(directory_item) qui, en plus d'alimenter la liste de directory_item, a la responsabilité d'informer l'objet directory_item en question qui est son répertoire parent (via directory_item::parent()). Cela donne une double responsabilité, c'est peut-être là que ça péche ?
Attention c'est (un peu) C++0x :
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 class directory_item; class directory: public directory_item { public: void add(std::shared_ptr<directory_item> an_item); private: std::list<std::shared_ptr<directory_item>> m_items; }; class directory_item { public: void parent(std::weak_ptr<directory> parent_directory); private: std::weak_ptr<directory> m_parent_directory; };
Code:

1 2 3 4 5 6 7 8 9 10 11 12 13 void directory::add(std::shared_ptr<directory_item> an_item) { m_items.push_back(an_item); an_item->parent(???); //que mettre ici ? } void directory_item::parent(std::weak_ptr<directory> parent_directory) { m_parent_directory = parent_directory; }
La ligne qui me pose problème est en rouge.
En premier, j'ai essayé this qui évidemment ne compile pas.
Ensuite, j'ai utilisé std::shared_from_this(), mais il semblerait que le weak_ptr directory_item::m_parent_directory devienne immédiatement invalide juste après son affectation. Le compteur de référence du shared_ptr résultant de std::shared_from_this() doit tomber à zéro… pourtant le directory n'est pas détruit (peut-être que le deleter du shared_ptr est sans effet ? http://www.boost.org/doc/libs/1_36_0...es.html#static).

Quoi qu'il en soit, il semblerait que le weak_ptr à passer en paramètre à directory_item::parent(directory) doive être créé à partir d'un shared_ptr persistant.
Dans ce cas, la seule solution qui me reste serait de créer une fonction libre ayant la signature ci-dessus, que l'objet directory pourra appeler pour obtenir un shared_ptr persistant sur lui-même à partir de *this :
Code:

1 2 std::shared_ptr<directory> find_directory(filesystem&, const directory&);
Est-ce que ma façon de penser est correcte ?
Si oui, est-ce que la solution que je donne est la bonne ?

Je ne vois pas a priori de problème à passer shared_from_this() à la fonction parent. Peux-tu montrer ton code dans ce cas ?

Un cas qui pourrait poser problème, c'est si add est appelé depuis le constructeur de directory. En effet, à ce moment, l'objet n'a pas encore fini d'être créé, et donc il n'a pas encore été affecté au pointeur intelligent qui va le gérer, et donc shared_from_this ne fonctionne pas bien. Dans ce cas, la solution passe souvent par une factory qui dans ton cas ressemblerait à :
Code:

1 2 3 4 5 6 shared_ptr<Directory> createDirectory(/*args*/) { shared_ptr<Directory> result(new Directory(/*args*/); // Pas de lecture du contenu ici result->readContent(); // Ici, add marchera return result; }
J'en parle un peu dans mon article sur le sujet : http://loic-joly.developpez.com/tuto...mart-pointers/

Citation:

Un cas qui pourrait poser problème, c'est si add est appelé depuis le constructeur de directory.

Pas de problème de ce côté, ce n'est pas le cas.

Citation:

J'en parle un peu dans mon article sur le sujet : http://loic-joly.developpez.com/tuto...mart-pointers/

Oui, c'est en lisant ton très chouette article que j'ai été convaincu de l'utilité des smart pointers ;)

Citation:

Peux-tu montrer ton code dans ce cas ?

En réalité, il s'agit d'une classe namespace_ (comme un namespace C++) et non d'une classe directory dans mon code. Mais le principe est le même :

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
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
 
#ifndef CPPPARSER_PROGRAM_MODEL_NAMESPACE_H
#define CPPPARSER_PROGRAM_MODEL_NAMESPACE_H
 
#include <string>
#include <vector>
#include <memory>
#include "namespace_item.h"
 
namespace cppparser { namespace program_model
{
 
class class_;
class enum_;
class typedef_;
 
/**
Represents a C++ namespace.
*/
class namespace_: public namespace_item, public std::enable_shared_from_this<namespace_>
{
    public:
        /**
        Creates an anonymous namespace. Equivalent to namespace_("").
        */
        namespace_();
 
        /**
        Creates a named namespace.
        @param name the namespace's name.
        */
        explicit namespace_(const std::string& name);
 
        /**
        Destructor.
        */
        ~namespace_();
 
        /**
        @return the name of the namespace.
        Anonymous namespaces return an empty string.
        */
        const std::string&
        name() const;
 
        /**
        @return the full name of the namespace, including all parents (e.g. ::foo::bar).
        */
        std::string
        full_name() const;
 
        /**
        @return true if the namespace is the global one, false otherwise.
        */
        bool
        is_global() const;
 
        /**
        Check whether an already declared symbol has the given name.
        @param name the name of the namespace to search.
        @return true if an already existing namespace has the same name, false otherwise.
        */
        std::shared_ptr<namespace_>
        find_namespace(const std::string& name) const;
 
        /**
        @return the item list of the namespace, i.e. the list of classes, function, etc.
        */
        const std::vector<std::shared_ptr<namespace_item>>&
        items() const;
 
        /*void
        add(std::shared_ptr<namespace_item> a_namespace_item);*/
 
        void
        add(std::shared_ptr<namespace_> a_namespace);
 
        ///@todo find better than that dirty trick
        void
        shared_this(std::shared_ptr<namespace_> ptr);
 
    private:
        std::string m_name;
        std::shared_ptr<namespace_> m_shared_this;
        std::vector<std::shared_ptr<namespace_item>> m_items;
        std::vector<std::shared_ptr<namespace_>> m_namespaces;
};
 
}} //namespace cppparser::program_model
#endif

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
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
 
#include <iostream>
#include <algorithm>
#include <stdexcept>
#include "class_.h"
#include "enum_.h"
#include "typedef_.h"
 
#include "namespace_.h"
 
namespace cppparser { namespace program_model
{
 
namespace_::namespace_()
{
}
 
namespace_::namespace_(const std::string& name):
    m_name(name)
{
}
 
namespace_::~namespace_()
{
}
 
const std::string&
namespace_::name() const
{
    return m_name;
}
 
std::string
namespace_::full_name() const
{
    std::string full_name;
 
    if(!is_global())
    {
        full_name = parent().lock()->full_name() + "::";
    }
    full_name += m_name;
 
    return full_name;
}
 
bool
namespace_::is_global() const
{
    return !has_parent();
}
 
std::shared_ptr<namespace_>
namespace_::find_namespace(const std::string& name) const
{
    ///@todo use STL algo. instead
    for
    (
        std::vector<std::shared_ptr<namespace_>>::const_iterator i = m_namespaces.begin();
        i != m_namespaces.end();
        ++i
    )
    {
        std::shared_ptr<namespace_> n = *i;
        if(n->name() == name)
        {
            return n;
        }
    }
 
    return std::shared_ptr<namespace_>(); //return a null pointer if no namespace found
}
 
 
 
const std::vector<std::shared_ptr<namespace_item>>&
namespace_::items() const
{
    return m_items;
}
 
void
namespace_::add(std::shared_ptr<namespace_> a_namespace)
{
    //check whether no already existing namespace has the same name
    if(find_namespace(a_namespace->name()))
    {
        throw std::runtime_error(full_name() + " already contains a namespace named \"" + a_namespace->name() + "\".");
    }
 
    //tell namespace that we (i.e. this) are its parent
    a_namespace->parent(shared_from_this()); //si on laisse cette ligne, on a le droit à un std::bad_weak_ptr à l'exécution
    //a_namespace->parent(shared_this); //par contre, si on met ceci à la place, tout se passe correctement… mais c'est un peu sale, comme code
    a_namespace->shared_this(a_namespace); ///< @todo find better than that dirty trick
 
    //add namespace to private containers
    m_namespaces.push_back(a_namespace);
    m_items.push_back(a_namespace);
}
 
void
namespace_::shared_this(std::shared_ptr<namespace_> ptr)
{
    m_shared_this = ptr;
}
 
}} //namespace cppparser::program_model

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
 
#ifndef CPPPARSER_PROGRAM_MODEL_NAMESPACE_ITEM_H
#define CPPPARSER_PROGRAM_MODEL_NAMESPACE_ITEM_H
 
#include <memory>
 
namespace cppparser { namespace program_model
{
 
class namespace_;
 
class namespace_item
{
    public:
        virtual ~namespace_item();
 
        bool
        has_parent() const;
 
        std::weak_ptr<namespace_>
        parent();
 
        const std::weak_ptr<namespace_>
        parent() const;
 
        void
        parent(std::weak_ptr<namespace_> parent);
 
    private:
        std::weak_ptr<namespace_> m_parent;
};
 
}} //namespace cppparser::program_model
 
#endif

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
 
#include <iostream>
#include "program.h"
 
#include "namespace_item.h"
 
namespace cppparser { namespace program_model
{
 
namespace_item::~namespace_item()
{
}
 
bool
namespace_item::has_parent() const
{
    return !m_parent.expired();
}
 
std::weak_ptr<namespace_>
namespace_item::parent()
{
    return m_parent;
}
 
const std::weak_ptr<namespace_>
namespace_item::parent() const
{
    return m_parent;
}
 
void
namespace_item::parent(std::weak_ptr<namespace_> parent)
{
    m_parent = parent;
}
 
}} //namespace cppparser::program_model

EDIT : J'ajoute aussi la classe program, contenant le namespace global.
Le namespace global étant créé statiquement, j'utilise un shared_ptr avec un deleter sans effet :

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
#ifndef CPPPARSER_PROGRAM_MODEL_PROGRAM_H
#define CPPPARSER_PROGRAM_MODEL_PROGRAM_H
 
#include <memory>
#include "namespace_.h"
 
namespace cppparser { namespace program_model
{
 
/**
Represents a C++ program.
A program can be either an executable or a library.
*/
class program
{
    public:
        program();
 
        /**
        @return the global namespace of the program.
        */
        std::shared_ptr<namespace_>
        global_namespace();
 
    private:
        namespace_ m_global_namespace;
        std::shared_ptr<namespace_> m_global_namespace_ptr;
};
 
}} //namespace cppparser::program_model
 
#endif

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
 
#include "program.h"
 
namespace cppparser { namespace program_model
{
 
namespace
{
struct null_deleter
{
    void operator()(void const *) const
    {
    }
};
}
 
program::program():
    m_global_namespace_ptr(&m_global_namespace, null_deleter())
{
    m_global_namespace.shared_this(m_global_namespace_ptr);
}
 
std::shared_ptr<namespace_>
program::global_namespace()
{
    return m_global_namespace_ptr;
}
 
}} //namespace cppparser::program_model

22/08/2008, 08h28
JolyLoic
Citation:

Envoyé par three minute hero

EDIT : J'ajoute aussi la classe program, contenant le namespace global.
Le namespace global étant créé statiquement, j'utilise un shared_ptr avec un deleter sans effet :

Ah, enfin un truc un peu louche dans ton code ;)

Je ne suis pas certain que ce soit lié, mais pourquoi ne pas avoir simplement un namespace global créé dynamiquement comme les autres, et dans ta classe program, une seul variable membre, de type std::shared_ptr<namespace_> ?
Code:

1 2 3 4 program::program(): m_global_namespace_ptr(new(namespace_)) { }
Autre point, sans rapport avec ton problème, pourquoi ne pas faire retourner à namespace_item::parent() un shared_ptr, tout en gardant un weak_ptr comme donnée membre ? Ca simplifierait probablement l'utilisation de cette fonction.
22/08/2008, 10h27
three minute hero

Citation:

Envoyé par JolyLoic

Ah, enfin un truc un peu louche dans ton code ;)

Je ne suis pas certain que ce soit lié, mais pourquoi ne pas avoir simplement un namespace global créé dynamiquement comme les autres, et dans ta classe program, une seul variable membre, de type std::shared_ptr<namespace_> ?

Parc'queeeee ! C'est pas optimisé comme ça ! Et ça n'a rien d'une optimisation prématurée. On est pas en java :roll:.
Bien, plus sérieusement, je teste ça immédiatement.
(…)
Hélas cela ne change rien. De plus, que le namespace global soit créé dynamiquement ou statiquement, si j'utilise le système que j'ai mis en place le temps de régler ce problème, tout fonctionne bien :

Le propriétaire de chaque namespace passe au namespace une copie d'un shared_ptr vers lui-même (le namespace), voir namespace_::shared_this() et m_shared_this.
Ainsi, lors de l'exécution de namespace_::add(), où le namespace doit passer un pointeur vers lui-même à son enfant (le namespace_item, ou dans mon code le namespace_ tout court, car je vais surcharger add() pour chaque type d'item), le namespace n'a plus qu'à passer m_shared_this à son enfant (le fameux shared_ptr persistant dont j'ai parlé dans mon premier post) en lieu et place où j'aimerais plutôt écrire shared_from_this().
Ça renforce l'hypothèse que le problème vient de shared_from_this(). Si je laisse shared_from_this(), j'ai immanquablement droit à une exception de type bad_weak_ptr.

Citation:

Envoyé par JolyLoic

Autre point, sans rapport avec ton problème, pourquoi ne pas faire retourner à namespace_item::parent() un shared_ptr, tout en gardant un weak_ptr comme donnée membre ? Ca simplifierait probablement l'utilisation de cette fonction.

Il me semblait qu'un shared_ptr généré à partir d'un weak_ptr devait rester le plus temporaire possible. D'autant plus que la fonction pour obtenir ce shared_ptr s'appelle weak_ptr::lock(). Ça sonne un peu mutex, ça me donne pas envie de créer un shared_ptr persistant à partir de ça !
22/08/2008, 10h47
JolyLoic

Pour ton problème, j'essayerais de regarder ça ce soir...

Pour le retour de fonction, il est justement temporaire. Est-ce que tu vois un cas d'utilisation où tu peux vouloir appeler la fonction parent sans regarder ce que vaut justement ce parent ?
24/08/2008, 15h22
three minute hero
Bien bien bien.
J'ai repris le problème ce matin.

Déjà, j'ai fait en sorte que namespace_ store un weak_ptr vers lui-même, et non un shared_ptr. Les objets namespace_ n'étaient jamais détruits, forcément, vu que le compteur était au minimum à 1.
Ça, c'est déjà une bonne chose de faite, et en plus ça nous rapproche de ce que fait enable_shared_from_this en interne (voir ici).

Maintenant que le problème est encore plus ciblé, j'ai pu un peu mieux relire la page de la doc de boost concernant enable_shared_from_this, où il est écrit très clairement que :

Citation:

Requires: enable_shared_from_this<T> must be an accessible base class of T. *this must be a subobject of an instance t of type T . There must exist at least one shared_ptr instance p that owns t.

Au début j'avais pris ça pour « l'objet appelant shared_from_this() sur lui-même doit être holdé par un autre objet via un shared_ptr, n'importe lequel ». En fait, il faut comprendre « l'objet appelant shared_from_this sur lui-même doit être holdé par un autre objet via un shared_ptr créé lui aussi à partir de shared_from_this() », nuance.
C'est ça qui garanti que le shared_ptr créé par shared_from_this() est persistant, vu qu'une copie partageant le même compteur traîne dans un autre objet.

Voilà pourquoi dans la doc de boost (voir le premier lien du post) ils créent leur objet dans une fonction membre statique factory au lieu d'un constructeur (ce que tu fais également dans ton article, Loïc).

Tout cela explique pourquoi mon code ne fonctionnait pas.
Si un objet de type directory (reprenons cet exemple) envoie un shared_ptr de lui-même (créé par shared_from_this) à son item, comme ici… :
Code:

1 2 3 4 5 6 7 void directory::add(std::shared_ptr<directory_item> an_item) { m_items.push_back(an_item); an_item->parent(shared_from_this()); }
… et que l'objet de type directory_item store un weak_ptr de son directory parent… :
Code:

1 2 3 4 5 6 7 8 9 10 class directory_item { public: void parent(std::weak_ptr<directory> parent_directory); private: std::weak_ptr<directory> m_parent_directory; };
… ça ne peut pas fonctionner, car dés que directory_item::parent() a terminé son exécution, le shared_ptr créé par shared_from_this est détruit, donc le weak_ptr devient immédiatement invalide.

Je poste tout de suite pour ne pas perdre ce long message, j'éditerai pour ajouter des informations résultant des quelques essais que je m'aprête à faire.

Je ne sais pas si tu avais depuis longtemps compris les choses comme ça, Loïc, mais je pense qu'un petit encart en plus dans ton article pourrait aider bien des gens.
24/08/2008, 16h39
three minute hero
Aaaaaahh, ça y est, j'ai compris !

En fait, en théorie ça devrait marcher, puisque, comme on peut le lire dans la doc de boost :

Citation:

Note that you no longer need to manually initialize the weak_ptr member in enable_shared_from_this. Constructing a shared_ptr to impl takes care of that.

Voilà pourquoi ça ne fonctionne pas en pratique.
J'ai créé mes objets directement avec std::make_shared() :

Code:

std::shared_ptr<directory> new_directory = std::make_shared<directory>(directory_name);

Cette expression semble être, en gros, équivalente à l'expression ci-dessous, mais en mieux car plus rapide et expression-safe :

Code:

std::shared_ptr<directory> new_directory(new directory(directory_name));

Sauf que std::make_shared() utilise la move semantic (c'est une pure supposition que je fais ici, mais la suite du raisonnement se tient tellement que ça ne peut être que ça), ce qui fait qu'il n'est disponible qu'avec C++0x.
Or, GCC, notre très cher compilateur adoré, ce petit coquinou, n'implémente pour le moment pas totalement les rvalue references. À l'heure où j'écris ce message, on peut en effet lire sur cette page que :
Code:

1 2 3 4 Language Feature Proposal Available in GCC? Rvalue references N2118 GCC 4.3 Rvalue references for *this N2439 No
No !!
À mon avis, la fonctionnalité rvalue references for *this devrait être utilisée par la surcharge/spécialisation de std::make_shared() destinée aux classes héritant de std::enable_shared_from_this. Sauf qu'étant donné que cette fonctionnalité n'est pas encore implémentée, cette spécialisation n'existe pas encore. Résultat : le weak_ptr de std::enable_shared_from_this n'est pas initialisé, et on ne peut s'en servir de façon transparente, CQFD.

Finalement, si j'utilise explicitement l'opérateur new au lieu de std::make_shared(), mon code fonctionne comme prévu, sans même avoir à se poser de question sur l'implémentation de std::enable_shared_from_this.

Conclusion n°1 : Le dimanche rend intelligent.
Conclusion n°2 : Ça m'apprendra à utiliser une implémentation bêta d'un langage encore sur brouillon :lol:

Merci à toi, Loïc, pour ton coup de main.

De rien... Et désolé de t'avoir oublié alors que j'avais dit que je regarderais de plus près chez moi... :sm:

J'ai quelques doutes sur ton explication, les R-Value ref dans make_shared ayant pour moi le seul but de faire un perfect forwarding des arguments du constructeur.

Maintenant, il est vrai que je n'ai pas encore utilisé make_shared, et donc que je ne connais pas bien ses interactions avec shared_from_this. Si j'ai bien compris, tu dis que le code suivant ne marche pas :
Code:

1 2 3 4 5 6 7 8 9 10 11 class A : enable_shared_from_this<A> { shared_ptr<A> getIt() {return shared_from_this();} }; int main() { shared_ptr<A> p (make_shared<A>()); //1 weak_ptr<A> w (p->getIt()); assert(!w.expired()); }
Et que ça marcherait si on remplaçait 1 par :

Code:

shared_ptr<A> p (new A); //1

C'est bien ça ?

Si oui, c'est bien ce que j'appellerais un bug...

24/08/2008, 17h42
three minute hero

Pas de quoi t'excuser, enfin… tu aurais bien pu ne même pas me répondre.

Ouch, j'aurais jamais réussi à synthétiser le problème à ce point. Mais oui, c'est exactement ça. J'ai testé le code, et effectivement, l'utilisation de shared_from_this() lance un std::bad_weak_ptr (même pas besoin de l'assertion).
24/08/2008, 23h38
three minute hero

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36949
C'était bien un bug ;)
Il sera résolu pour la 4.4.
25/08/2008, 06h12
epsilon68

si j'ai bien compris il y a un bug dans GCC libstd++
mais pourquoi tu n'utilises pas Boost ?
25/08/2008, 11h28
three minute hero

std::make_shared() ne fait pas partie de boost, c'est du C++0x (edit : enfin, je crois…).

Puis je préfère utiliser la libc++ de C++0x, c'est pour un projet perso donc c'est pas bien problématique, et ça permettra de ne pas avoir à modifier le code dans le futur.
25/08/2008, 11h34
JolyLoic

Ils en ont ajouté un dans boost 1.36 je crois (en tout cas, je l'ai vu dans le svn). Sur un compilo qui aurait les rvalue reference et les variadic templates, c'est implémenté comme la norme, a priori, sur les autres, c'est implémenté par approximation.

Par contre, j'avais cru voir que sur un compilo qui aurait TR1, boost serait un simple forward vers ce compilo (sauf que make_shared n'est pas dans TR1, ça devient un peu sac de nœuds...).

Donc, pour avoir l'implémentation boost, il faudrait un compilo qui aurait les rvalue reference et les variadic templates, mais pas TR1. Est-ce que ça existe ?
25/08/2008, 13h46
epsilon68

la norme C++ 0x est vraiment allechante, surtout les rvalues,
mais quand je pense au nombre d'année necessaire pour pouvoir utiliser les nouvelles fonctionnalités ... pfffou j'en ai le vertige.

bon enfin j'y crois, je ne suis pas désespéré :-)
25/08/2008, 15h15
three minute hero

Tu peux d'ores et déjà utiliser les rvalue references avec GCC sous Linux.
Sous Windows, MingGW-gcc4.3 est en version alpha, alors faut voir.

Personnellement, utilisant Spirit (pour le projet évoqué dans ce topic), auto me fait bien envie, aussi.