Discussion :

[whityranger]

Bonjour à tous,

Je viens à vous car je me heurte à un problème que je n'arrive pas à comprendre.
Je tente d'implémenter une classe singleton Template réutilisable ; en m'inspirant de ce que je trouve sur le net, j'arrive à une première implémentation (non encore thread-safe, mais c'est pas la question), avec des pointeurs classiques, qui marche :

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
62
63
64
65
66
67
68
 
#ifndef DEFINE_SINGLETON
#define DEFINE_SINGLETON
 
#include <iostream>
 
 
// Classe de design pattern Singleton
 
template <typename T>
 
class Singleton {
 
protected :
 
  // Pointeur vers l'instance unique
  static T* singletonPtr;
 
  // Constructeur et destructeur privés pour éviter l'appel depuis l'extérieur.
  Singleton () {}
  ~Singleton() {}
 
public :
 
  // Méthode statique pour accéder à l'instance unique du Singleton ; celle-ci est
  // construite si elle n'était pas encore allouée.
  static T* getInstance() {
    if ( singletonPtr == nullptr ) singletonPtr = new T;
    return singletonPtr;
  }
 
  // Méthode statique pour détruire l'instance unique de T
  static void kill() {
    if ( singletonPtr != nullptr) delete singletonPtr;
    singletonPtr = nullptr;
  }    
 
};
 
// définition du singleton statique, intialisé sur nullptr
template <typename T>
T* Singleton<T>::singletonPtr = nullptr;
 
// Classe test utilisant la classe ci-dessus
 
class Test : public Singleton<Test> {
 
  friend class Singleton<Test>;
 
private :
 
  // Constructeur et destructeur private pour éviter l'instanciation/destruction
  Test() {}
  ~Test(){}
 
  // Constructeur de copie et opérateur d'affectation interdits
  Test(const Test& other) = delete;
  Test operator=(const Test& other) = delete;
 
public :
 
  // une méthode quelconque
  void doSomething() { std::cout << "doSomething" << std::endl; }
 
};
 
 
#endif // ndef DEFINE_SINGLETON

et le main pour tester :

Code :

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

1
2
3
4
5
6
7
8
9
 
#include <iostream>
#include "SingletonSP.hpp"
 
int main() {
  Test::getInstance()->doSomething();
  Test::kill();
  return 0;
};

Ceci compile et marche parfaitement.

Ensuite, j'essaie une version avec des shared_ptr :

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
62
63
64
65
66
 
#ifndef DEFINE_SINGLETON
#define DEFINE_SINGLETON
 
#include <iostream>
#include <memory>
 
 
// Classe de design pattern Singleton
 
template <typename T>
class SingletonSP {
 
protected :
  // Pointeur vers l'instance unique
  static std::shared_ptr<T> singletonPtr;
 
  // Constructeur et destructeur privés pour éviter l'appel depuis l'extérieur.
  SingletonSP () {}
  ~SingletonSP() {}
 
public :
 
  // Méthode statique pour accéder à l'instance unique du Singleton ; celle-ci est
  // construite si elle n'était pas encore allouée.
  static std::shared_ptr<T> getInstance() {
    if ( !singletonPtr )  singletonPtr = std::make_shared<T>();
    return singletonPtr;
  }
 
  // kill() n'est a priori plus nécessaire, puisque singletonPtr sera détruit à la fin du scope
  // global, désalouant automatiquement l'objet pointé.
 
};
 
// la définition du singleton statique, non initailisée car encapsulée.
template <typename T>
std::shared_ptr<T> Singleton<T>::singletonPtr;
 
// Classe test utilisant la classe ci-dessus
 
class Test : public SingletonSP<Test> {
 
  friend class Singleton<Test>;
  friend std::shared_ptr<Test> std::make_shared<Test>(); // friend pour avoir accès au constructeur
  friend class std::shared_ptr<Test>; // friend pour avoir accès au destructeur
 
private :
 
  // Constructeur et destructeur private pour éviter l'instanciation/destruction
  Test() {}
  ~Test(){}
 
  // Constructeur de copie et opérateur d'affectation interdits
  Test(const Test& other) = delete;
  Test operator=(const Test& other) = delete;
 
public :
 
  // une méthode quelconque
  void doSomething() { std::cout << "doSomething" << std::endl; }
 
};
 
 
#endif // ndef DEFINE_SINGLETON

et le main à peine modifié :

Code :

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

1
2
3
4
5
6
7
8
 
#include <iostream>
#include "SingletonSP.hpp"
 
int main() {
  Test::getInstance()->doSomething(); // <-ligne 9
  return 0;
};

Et là, la belle erreur de compilation, qui en clair me dit qu'à la ligne 9 je fais appel aux constructeur et destructeur de Test, ce qui est interdit car ils sont private.
L'erreur exacte du compilo est, sans toutes les inclusions de la stl (que j'ai remplacées par (...) pour rendre le truc lisible) :
(...)
SingletonSP.hpp:28:62: required from ‘static std::shared_ptr<_Tp1> SingletonSP<T>::getInstance() [with T = Test]’
main.cpp:6:9: required from here
SingletonSP.hpp:52:3: error: ‘Test::Test()’ is private
Test() {}
(...)
main.cpp:9:2: required from here
SingletonSP.hpp:53:3: error: ‘Test::~Test()’ is private
~Test(){}
(...)

Voilà, j'imagine qu'il s'agit d'une subtilité dans l'utilisation des shared_ptr que je ne vois pas, mais justement je ne la vois pas...
Précision, j'utilise g++ 4.8.1 avec les options -Wall, -Wextra et -std=c++11

Merci d'avance,

whityranger

[jabbounet]

Bonsoir

je ne pense pas que le concept de singleton et de shared_ptr puisse faire bon ménage.

Comme pour la plupart des conteneurs de la stl (probablement presque tous) l'un des pré-requis pour leur utilisation est de leur fournir des objets qui puissent-être construit par copie, Or c'est justement ce que l'on souhaite éviter avec le singleton.

si on lit attentivement la doc d'origine
http://www.boost.org/doc/libs/1_55_0...lt_constructor

Every shared_ptr meets the CopyConstructible, MoveConstructible, CopyAssignable and MoveAssignable requirements of the C++ Standard Library, and can be used in standard library containers. Comparison operators are supplied so that shared_ptr works with the standard library's associative containers.

d'ailleur le compilateur te dit clairement que pour ton exemple de code shared_ptr ne peut pas accéder ni au constructeur Test::Test(), ni au destructeur Test::~Test() car ces dernier sont privés....

[koala01]

Salut,

Tout comme jabbounet, j'estime que le shared_ptr n'est sans doute pas adapté, mais pour une autre raison : shared_ptr est un pointeur partagé.

Autrement dit, shared_ptr a pour but de s'assurer que la mémoire allouée à un objet sera correctement détruite lorsque le dernier objet qui l'utilise est détruit. Ce n'est pas le but du singleton qui est de fournir un objet dont on est sur qu'il existera toujours! Ce n'est donc pas ce qu'il te faut.

Le but du singleton est de s'assurer qu'il n'y aura à tout instant qu'une seule et unique instance de ton objet. Or, si tu veux un pointeur unique, ce serait plutôt std::unique_ptr qu'il te faudrait.

Cela prendrait une forme proche 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
template <typename T>
class Singleton{
    public:
        /* pour respecter ton code original(*) */
        static T* instance(){
           /* rendu obligatoire à cause de la présence de kill */
           if(! instance_.get())
               instance_.reset(new T);
            return instance_.get();
        }
        /* pour respecter ton code original (*) */
        static void kill(){
            instance_.reset();
        }
};
static std::unique_ptr<T> Singleton::instance_= std::unique_ptr<T>(new T);

(*) Typiquement, un singleton est construit au début de l'exécution du programme et détruit lorsque le programme s'achève. Et, lorsque le programme s'achève, toute la mémoire allouée à ce programme est automatiquement libérée par le système.

Mais un singleton a aussi pour but de s'assurer que nous accéderons toujours au même objet, quel que soit le nombre de fois que nous tenterons d'y accéder.

Dés lors, on peut décemment se poser la question : Pourquoi donner à l'utilisateur l'occasion de détruire l'objet"

Ceci dit, le problème vient de ce à quoi ton singleton accorde son amitié.

Je m'explique : Dans ta version originale, tu déclares la classe Singleton<T> amie du singleton "concret". C'est ce qu'il faut faire, parce que c'est Singleton<T> qui va essayer d'appeler le constructeur de ton singleton concret.

Le problème, c'est que dans ta version avec un shared_ptr, ce n'est pas Singleton<T> qui va faire appel au constructeur : c'est std::make_shared. Or, std::make_shared n'est pas une fonction membre de Singleton<T> : C'est une fonction libre.

Tu en arrive donc à une situation dans laquelle ton singleton accorde son amitié à "quelque chose" (comprends : Singleton<T>) qui n'en a que faire, mais qui n'accorde pas son amitié à ce qui en a réellement besoin, à savoir std::make_shared.

Tu as donc deux solution : Soit tu crées toi-même le shared_ptr, soit tu veilles à ce que ton singleton concret accorde son amitié à ce qui en a vraiment besoin

En outre, il est tout à fait possible d'éviter le recours au pointeur et à l'allocation dynamique de la mémoire lorsque tu envisages (l'anti) le patern singleon. Tu peux parfaitement définir une variable statique directement dans la fonction instance(), en veillant alors à renvoyer une référence au lieu d'un pointeur.

Cela prendrait 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
/** le concept d'objet non copiable
  *
  * j'aime disposer d'une classe qui me permette de représenter
  * le concept d'objet non copiable.  Cela permet de rendre la
  * notion plus "conceptuelle" en plus de faciliter l'écriture ;)
  *
  * @note Cette classe est destinée à intervenir dans une relation d'héritage privé
  */  
struct NonCopyable{
    NonCopyable(NonCopyable const &) = delete;
    NonCopyable & operator = (NonCopyable const &) = delete;
};
/* La classe de base pour tous les singleton
 *
 * Le but du singleton est d'assurer l'unicité de l'objet.
 * Il ne doit donc être ni copiable ni affectable
 */
template <typename T>
SingletonByRef : private NonCopyable{
    public:
        T & instance(){
             static T obj;
             return obj;
         }
};
/** Mon singleton concret
   *
   * Comme SingletonByRef<T> n'est pas copiable ni affectable,
   * cette classe ne l'est pas non plus du fait de l'héritage
class MySingleton : public SingletonByRef<T>{
    friend class SingletonByRef<T>;
    public:
        void doSomething(){}
    private:
        MySingleton(){}
        ~MySingleton(){}
};

qui pourrait être utilisée sous une forme proche de

Code :

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

1
2
3
4
int main(){
    MySingleton obj;
    obj.doSomething();
}

Cette manière de faire n'a que des avantages :

1. Tu gardes la syntaxe d'accès "classique" au lieu d'utiliser la syntaxe dédiée aux pointeurs (bon, c'est pas le plus gros avantage, mais c'en est quand même un )
2. Tu évites le recours au pointeurs et tous les problèmes connexes
3. Tu es sur que l'instance de ton singleton concret sera créée en temps opportun et qu'elle sera d'office détruite à la fin de l'exécution du programme
4. Tu n'a pas besoin d'exposer une fonction kill qui permet à l'utilisateur de détruire ton singleton alors qu'il n'a simplement pas à le faire

Enfin, je ne peux que m'insurger une fois de plus contre l'usage de cet anti pattern!!!

Il faut bien te dire qu'une variable statique n'est jamais qu'une variable globale habilement déguisée qui flotte sur un océan de mystère et que les variables globales, C'EST MAL.

En plus, un singleton est, par définition, une variable qui est disponible PARTOUT. Si tu as besoin d'une telle variable, c'est très certainement que tu as un gros problème de partage des tâches dans le sens où tu permet à trop de choses différentes de faire la même chose. Avec, comme corollaire le fait que, quand tu seras confronté à un bug, tu devras parcourir tout ton code pour trouver tous les endroits où une action particulière est entreprise afin de les corriger... Avec la quasi certitude d'en oublier systématiquement.

Le meilleur moyen de n'avoir qu'un instance d'un objet est toujours de veiller n'en créer qu'une

[whityranger]

Merci pour vos deux réponses : celle de jabbounet pour l'aspect compréhension technique, celle de koala01 pour la solution alternative plus robuste.

Je rebondis dessus : je suis bien entendu conscient du fait que le singleton est une variable globale, et que ces dernières sont souvent source de complications au débuggage. Cependant, j'ai tendance à m'en servir uniquement pour deux types de choses : un conteneur gérant des options connues uniquement à l’exécution (en général une struct initialisée en début de programme, c'est quelque chose que j'utilise surtout dans mes codes de calcul scientifique, pour gérer l'abondance des informations affichées dans le log par exemple), et les gestionnaires de ressources. En l'occurrence, il s'agit d'un jeu et de gestionnaires de ressources (pour l'instant textures, mais à venir sons etc...).

Donc la question que je me pose est : y a-t-il mieux ? Je n'en ai pas l'impression. Les classes qui utilisent ces ressources n'ont pas à savoir comment es autres classes les utilisent, donc il me semble logique d'avoir un gestionnaire externe. Avoir plusieurs gestionnaires serait un gâchis de mémoire. Et en déclarer un unique local au début de main pour le passer en paramètre supplémentaire à TOUTES les classes apparaissant ensuite et susceptibles de l'utiliser ou de contenir des classes susceptibles de l'utiliser me semble affreusement lourd pour pas grand-chose, d'autant plus que l'opérateur de résolution de portée permet de savoir quand on joue avec une variable globale singleton bien plus simplement qu'avec une variable globale simple.

Bref, si vous voyez plus simple, je suis preneur...

[koala01]

Et moi je vais te poser une autre question : Quelles sont les classes qui ont vraiment besoin d'avoir accès à toutes les informations que contiennent tes singletons

Et si tu me répond "toutes", tu passes par la fenêtre Je te suggère d'aller faire un petit tour vers une discussion qui devrait t'aider à comprendre mon idée. Après tout, elle est récente et de nombreuses voies sont explorées

[whityranger]

Alors, je n'ai pas encore décortiqué la discussion que tu as mise en lien, mais en la lisant en diagonale, j'ai l'impression que ne ressortent que quelques grands principes applicables dans mon cas, tels que "une classe doit avoir un seul et unique but bien défini", que je m'efforce de respecter (même si je ne le fais sans doute pas parfaitement... ). Le fait est qu'il ne s'agit pas ici de gérer une config dont les éléments accessibles doivent être définis avec soin, ni un transfert d'informations d'une source quelconque vers la classe qui se chargera de les traiter ; il s'agit de s'assurer qu'on ne charge en mémoire une et une seule fois des ressources partagées, et en l'occurrence un seul type de ressource (histoire de bien séparer les responsabilités) : les textures.

Donc là-dessus, se pose ta question :

Quelles sont les classes qui ont vraiment besoin d'avoir accès à toutes les informations que contiennent tes singletons ?

Et la réponse n'est pas "toutes", mais dans ce cas pas loin. J'accorde volontiers que le gestionnaire des polices ne sera utile qu'aux classes de Menus (et pourrait donc être instancié comme un membre statique privé de la classe mère Menu, par exemple), que le gestionnaire des sons devra être limité à quelques classes sur lesquelles je ne me suis pas encore penché... Et dans ce cas, ok.
Mais pour le manager de Textures, ben celles-ci sont utilisées par les menus (pour les images de fond), par les personnages, les objets à l'écran, la map, l'interface de jeu... Toutes classes qui sont assez nombreuses et n'ont pas grand-chose en commun (sauf les personnages et objets, qui dérivent d'une même classe mère Element dans ma conception actuelle).
Je vais même plus loin : les Element n'ont pas vraiment besoin de ces Textures, ce sont des objets internes gérant l'affichage (séparation des tâches) appelés SpriteManager qui s'en servent, et ils ne s'en servent qu'à leur construction (pas besoin de garder une variable membre associée donc). Donc les Element n'ont même pas besoin eux-même de TextureManager, et il faudrait le passer à leur constructeur juste pour le transmettre à qui de droit ? Ça ne me semble pas logique comme "séparation des tâches".

Donc :
1. soit j'instancie un Manager unique et je transmet une référence sur mon Manager dans tous les constructeurs de toutes les classes susceptibles de les utiliser ; ça implique de le passer en argument du constructeur des Element uniquement pour le passer ensuite au constructeur des SpriteManager... et ça me semble ne pas être le job de Element de gérer les ressources. C'est peut-être plus propre, mais personnellement je trouve ça lourd si c'est juste utilisé à la construction.
2. soit je place TextureManager::getInstance() en private et je déclare en amies SpriteManager (comme ça les autres n'y ont pas accès), mais aussi tous ceux qui ont besoin de ça parce qu'elles n'utilisent pas les SpriteManager en interne (donc la Map, et toutes les classes filles de Menu, puisque l'amitié n'est pas héritée -> au secours )
3. soit je retravaille les Menus pour qu'ils utilisent les SpriteManager en interne pour faire la 2. et n'avoir en friend que SpriteManager et Map -> déjà mieux, mais ça me semble quand même rustine ; à savoir que pour des raisons d'optimisation, je préfère gérer Map sans SpriteManager mais de façon personnalisée
4. soit je garde un singleton global et j'essaie de faire confiance à mon équipe de dev...

J'ai l'impression que tu trouveras la 1. plus propre, mais je pense que cela alourdit l'écriture du code pour pas grand-chose... à moins que tu ne me prouves le contraire :p

EDIT : je viens de penser à une 5e solution : puisque, à l'exception de Map, tous les objets affichés peuvent le faire par l'intermédiaire d'un SpriteManager, je pourrais placer TextureManager (non singleton) comme variable statique privée dans la classe SpriteManager, et déclarer les quelques méthodes (à vue de nez seulement deux) de Map qui ont besoin de TextureManager comme friend de la classe SpriteManager (pour accéder au TextureManager statique)... ainsi seuls les objets en ayant l'utilité ont accès à TextureManager, et en ont même connaissance... ça me semble plutôt pas mal, j'aimerais vos avis sur ces 5 idées