Discussion :

[grunk]

Bonjour à tous,

J'ai une class "App" qui me permet de stocker les pointeur "globaux" de mon appli qui ont besoin d'être accessible de partout :

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
class App
{
	public:
		static App& Get();
 
		void SetLogsManager(util::logs::Manager * manager)
		{
			mLogManager = manager;
		}
 
		logs::Manager * GetLogsManager()
		{
			return mLogManager;
		}
 
		void Clear()
		{
			delete mLogManager;
		}
 
		~App() {
		};
 
	private:
 
		/** Pointeur du singleton */
		static App* mInstance;
 
		/** Gestionnaire de journaux */
		logs::Manager * mLogManager;
 
		App();
 
		App(const App& rs);
 
		App& operator = (const App& rs)
		{
			if (this != &rs) {
				mInstance = rs.mInstance;
			}
 
			return *this;
		}
 
	};
}

Chaque pointeur à un getter et un setter. Au démarrage de l'appli les pointeurs sont passés à la classe de la façon suivante :

Code :

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

1
2
util::App& app = util::App::Get();
app.SetLogsManager(new logs::Manager());

Je peux alors ensuite accéder de partout à mes pointeurs :

Code :

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

util::App::Get().GetLogsManager()->write('hello');

La class App s'occupe de détruire tous les pointeurs à la fermeture. Le problème c'est que quelqu'un qui utilise cette classe ne sait pas forcément que la désallocation est gérée par la classe et peut donc potentiellement très bien détruire lui même les pointeur fournit par la classe.

Que pourrais je faire pour être certains que seul App soit le propriétaire des pointeurs et qu'elle soit la seul entité capable de les désallouer ?

Merci

[Bousk]

Salut,

tu peux
- stocker des unique_ptr dans App et fournir le pointeur interne via get en précisant qu'il ne faut pas les supprimer
- stocker des shared_ptr dans App et retourner un weak_ptr ou un const shared_ptr&
- garder ça comme ça en précisant qu'il ne faut pas les supprimer

Dans tous les cas, il est toujours possible de retrouver le pointeur interne et de foutre le bordel en le supprimant arbitrairement, mais dans ce cas il convient d'aller taper le fautif avec un clavier/écran/unité central

Ou bien fournir une class qui englobe le pointeur et fournit un operateur -> pour accéder à son contenu mais ne permet pas d'accéder au pointeur directement, sauf à modifier la classe pour ajouter un delete dessus, à priori aucun risque de ce côté. Mais c'est overkill

[grunk]

La méthode unique_ptr me plait bien

Code :

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

1
2
3
4
5
6
7
8
9
void SetLogsManager(unique_ptr<util::logs::Manager> manager)
{
    mLogManager = std::move(manager);
}
 
logs::Manager * GetLogsManager()
{
    return mLogManager.get();
}

Du coup est ce que j'aurais pas intérêt à faire un getter comme ceci ? :

Code :

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

1
2
3
4
logs::Manager & GetLogsManager()
{
    return *mLogManager;
}

Ca assurerais que personne ne puisse supprimer les pointeurs non ?

[Bousk]

Clairement, retourner une référence plutôt qu'un pointeur serait une sécurité supplémentaire. Mais pourquoi ne pas l'avoir déjà mise en place ?

[grunk]

Mais pourquoi ne pas l'avoir déjà mise en place ?

Erreur de débutant

Je viens de Java et PHP où (quasi) tout est référence , du coup j'ai tendance à oublier qu'en C++ faut le faire explicitement

[bacelar]

Moi, je ne vois vraiment pas l'intérêt de faire de la méthode write une méthode d'objet.
Avec une méthode statique, plus besoin de ce très vilain Singleton.

[Pyramidev]

Bonjour,

Quelques corrections au code de ton premier message :

Code :

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

1
2
3
4
5
void SetLogsManager(util::logs::Manager * manager)
{
	delete mLogManager; // sinon, au prochain appel de SetLogsManager, fuite mémoire
	mLogManager = manager;
}

Code :

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

1
2
3
4
5
6
void Clear()
{
	delete mLogManager;
	mLogManager = nullptr; // Sinon, GetLogsManager renverra un pointeur non nul vers
	                       // un objet qui n'existe plus.
}

Code :

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

1
2
3
~App() {
	delete mLogManager;
}

Mais il reste plusieurs problèmes. Par exemple, un utilisateur de la classe risque de ne pas savoir que App::SetLogsManager(util::logs::Manager * manager) prend en charge la mémoire de manager et risque d'écrire un code du genre :

Code :

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

1
2
3
4
5
6
util::App& app = util::App::Get();
std::unique_ptr<util::logs::Manager> manager = new logs::Manager();
	// Le destructeur de std::unique_ptr<util::logs::Manager> fera un delete.
app.SetLogsManager(manager);
	// Il y aura un autre delete.
	// Aïe ! Double delete !

Pour éviter cela, on peut changer la signature de la fonction en :

Code :

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

void SetLogsManager(std::unique_ptr<logs::Manager>&& manager)

et adapter le reste du code, mais c'est assez technique quand on débute en C++ car cela fait intervenir les rvalue reference.

A part ça, si App::GetLogsManager retourne une référence, c'est bien mais il faut alors ajouter un contrôle :

Code :

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

1
2
3
4
5
6
logs::Manager & GetLogsManager()
{
	if(!mLogManager)
		throw std::logic_error("Erreur. Gestionnaire de logs absent.");
	return *mLogManager;
}

Cela dit, avant de partir dans des solutions complexes, y a-t-il besoin que l'utilisateur de la classe change de gestionnaire de logs au cours du programme ?
Autrement dit, as-tu prévu d'appeler SetLogsManager au milieu de l'exécution du programme ou bien juste au début, pour initialiser ?
Dans le dernier cas, la solution la plus simple serait de supprimer SetLogsManager, de supprimer Clear et d'initialiser le gestionnaire de logs directement dans le constructeur de App.
Comme ça, d'un claquement de doigts, on évite tous les problèmes ci-dessus.

[koala01]

Bonjour à tous,

J'ai une class "App" qui me permet de stocker les pointeur "globaux" de mon appli qui ont besoin d'être accessible de partout :

Hummm... Déjà rien que cela, j'ai un doute...

La notion d'application est très certainement locale, mais, a priori, il devrait au moins y avoir moyen de "minimiser" la portée dans laquelle les différents pointeurs seront utilisés..

Code :

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

1
2
3
4
class App
{
	public:
		static App& Get();

Ohhh le beau singleton!!!

Tres beau, mais surement inutile

Code :

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

1
2
3
4
5
 
		void SetLogsManager(util::logs::Manager * manager)
		{
			mLogManager = manager; // et ci mLogManager != nullpr ???
		}

Beuurk ... Un mutateur, mais quelle horreur!!!
Pourquoi ne pas le créer directement dans le constructeur de ton application

Code :

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

1
2
3
4
5
 
		logs::Manager * GetLogsManager()
		{
			return mLogManager;
		}

Pourquoi revoyer l'élément par pointeur et non par référence

Code :

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

1
2
3
4
5
 
		void Clear()
		{
			delete mLogManager;
		}

je présume que le vrai problème de propriété se pose ici ?

Mais, n'oublie pas de remettre mLogManager à nullptr après destruction ,) (je reviendrai cependant sur ce point plus tard)

Code :

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

1
2
3
 
		~App() {
		};

Surprise : ta fonction clear tend à montrer que ton application est propriétaire du logManager, mais ton destructeur tend à prouver qu'il n'en est que l'utilisateur...

Faudrait essayer de mettre ces deux fonctions d'accord

Code :

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

1
2
3
4
5
 
	private:
 
		/** Pointeur du singleton */
		static App* mInstance;

pourquoi un pointeur en donnée membre statique pourquoi pas une donnée temporaire statique dans la fonction get() (qui devrait de toutes manière être la seule à avoir besoin de cette donnée)

Code :

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

1
2
3
 
		/** Gestionnaire de journaux */
		logs::Manager * mLogManager;

Soit c'est la fonction clear() (dans son implémentation actuelle) qui a raison, soit c'est le destructeur (dans son implémentation actuelle) qui a raison.

A priori, j'aurais tendance à croire que ta classe Application est belle et bien propriétaire du gestionnaire de log.

Du coup, je remplacerais ce pointeur nu par un unique_ptr, et je modifierais les différentes fonctions pour prendre ce point en compte :

Code :

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

1
2
3
4
5
 
		void SetLogsManager(util::logs::Manager * manager) // je ne suis pas convaincu que cette fonction soit utile
		{
			mLogManager.reset(manager);
		}

Code :

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

1
2
3
4
5
6
7
 
		logs::Manager & GetLogsManager()
		{
                        /* si le logmanager n'existe pas, c'est une erreur de programamtion */
                        assert(mLogManager.get() && "log manager not defined");
			return *(mLogManager.get());
		}

Code :

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

1
2
3
4
5
 
		void Clear()
		{
			mLogManager.reset();
		}

Code :

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

1
2
3
4
 
		~App() {
                /* il n'a plus rien à faire : std::unique_ptr se charge de tout */
		};

Code :

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

1
2
 
		App();

"logique" pour un singleton... si ce n'était pas un anti pattern, j'aurais rien à dire

Code :

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

1
2
3
4
5
6
7
8
9
10
11
 
		App(const App& rs);
 
		App& operator = (const App& rs)
		{
			if (this != &rs) {
				mInstance = rs.mInstance;
			}
 
			return *this;
		}

Quand on déclare le constructeur de copie et l'opérateur d'affectation dans l'accessibilité privée, c'est dans le but de les désactiver, et il n'y a aucune raison pour désactiver l'un mais pas l'autre...

Mais les déclarer dans l'accessibilité privée sans les définir peut poser problème au niveau des fonctions membres et au niveau de fonctions amies. Depuis C++11, on se contentera de les déclarer delete, sous la forme de

Code :

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

1
2
App(App const &) = delete;
App & operator(App const & ) = delete;

Chaque pointeur à un getter et un setter. Au démarrage de l'appli les pointeurs sont passés à la classe de la façon suivante :

Code :

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

1
2
util::App& app = util::App::Get();
app.SetLogsManager(new logs::Manager());

C'est une très mauvaise idée... C'est l'indice évident que tu réfléchis bien plus à ta classe en termes de données (tous les pointeurs qu'elle contient) qu'en termes de comportements (de services qu'elle doit rendre, d'ordres auxquelles elle doit obéir, de questions auxquelles elle doit répondre)

Il y a un tas de discussions au sujet des mutateurs sur le forum, je vais te laisser les rechercher par toi-même. Mais, de manière générale, un accesseur (getter) peut s'avérer utile s'il correspond bel et bien à un service que l'on est en droit d'attendre de la part de la classe, mais un mutateur est très rarement la chose à faire

Je peux alors ensuite accéder de partout à mes pointeurs :

Code :

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

util::App::Get().GetLogsManager()->write('hello');

Et, pourquoi d'abord essayer de récupérer ton logManager, au lieu de simplement invoquer une fonction "log(...)" au niveau de on application et en faisant en sorte que ce soit cette fonction qui dialogue avec le logManager

De cette manière, l'utilisateur n'a "même plus à savoir" que la mécanique sous-jacente est prise en charge par le logManager

La class App s'occupe de détruire tous les pointeurs à la fermeture. Le problème c'est que quelqu'un qui utilise cette classe ne sait pas forcément que la désallocation est gérée par la classe et peut donc potentiellement très bien détruire lui même les pointeur fournit par la classe.

Que pourrais je faire pour être certains que seul App soit le propriétaire des pointeurs et qu'elle soit la seul entité capable de les désallouer ?

1. Evite les accesseurs (getter) et fourni des services qui y feront appel de manière "transparente"
2. pour les accesseurs que tu ne peux pas éviter, fais en sorte de renvoyer une référence au lieu d'un pointeur (de toutes manières, ta classe App est sensée vivre plus longtemps que tout le reste