shared_ptr et composite

**three minute hero** · 21/08/2008, 10h31

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 :

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
 
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 :

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

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 :

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

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 ?

**JolyLoic** · 21/08/2008, 20h20

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 :

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

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/

**three minute hero** · 21/08/2008, 23h53

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.

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

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 namespace_.h :

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
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 namespace_.cpp :

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
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 namespace_item.h :

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
 
#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 namespace_item.cpp :

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
 
#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 program.h :

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
#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 program.cpp :

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
 
#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

**JolyLoic** · 22/08/2008, 08h28

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 :

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

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.

**three minute hero** · 22/08/2008, 10h27

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

.
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.

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 !

**JolyLoic** · 22/08/2008, 10h47

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 ?

shared_ptr et composite

C++

Vue hybride

Discussions similaires

Partager

Partager