Y'a pas de soucis, je suis au contraire ravi d'avoir l'avis d'un pro et ainsi de pouvoir améliorer mon code.Je préviens tout de suite : je suis critique, mais je ne pense pas à mal. Il y a plusieurs erreurs dans le code, et je vais les présenter de manière neutre (pas méchante, ni gentillette ; ton neutre, tout ça).
Par contre je crois qu'il y a méprise sur une partie du fonctionnement de mon logger. Je dis bien je crois, parce que je ne suis pas certain d'avoir parfaitement saisi toutes tes explications. Enfin de toute façon, s'il y a effectivement méprise, c'est de ma faute, j'aurais dû expliquer mon code. Mais je vais corriger cet écueil.
Mon logger fonctionne par groupes (comme celui de Koala). Chacun de ces groupes, identifiés par une chaîne de caractère, peut être configuré indépendamment des autres. Par exemple :
Il est aussi possible de ne pas spécifier de groupe du tout si cela n'est pas utile. Ne pas spécifier de groupe revient à manipuler le groupe L"". Pour chaque groupe, je peux spécifier un fichier de log unique, un niveau, ou l'activer/désactiver totalement. Je pense que tout monde avait compris ça, mais je tenais à être complet.
Code : Sélectionner tout - Visualiser dans une fenêtre à part rwl::Logger::enable( false, L"RWL" ); // Je désactive tous les logs effectués dans le groupe RWL
Les groupes et leur configuration sont gérés par la partie statique de mon logger. Il y a une deuxième partie qui est "dynamique", qui elle sert à logger proprement dit :
Lorsque je souhaite logger un message, je vais en fait construire une instance temporaire de mon Logger, qui sera automatiquement détruite à la fin de l'exécution de la ligne. C'est là que je crois qu'il y a méprise. Je ne récupère pas une instance stockée en statique dans mon Logger, j'en créé une!
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 /* ** La partie "dynamique" du logger */ public : enum class Level { trace, debug, info, warning, error, exception, fatal }; Logger( const std::wstring& name = L"" ); ~Logger( void ); std::wostringstream& log( Level level ); private : std::wstring m_name; std::wostringstream m_logbuffer; Level m_level; /* ** La partie statique destinée à configurer mes Loggers */ public : static void enable( bool enable, const std::wstring& name = L"" ); static void set_level( Level level, const std::wstring& name = L"" ); static void set_logfile( const std::wstring& file_path, const std::wstring& name = L"" ); private: class Logger_data { public : Logger_data( void ); Logger_data( const Logger_data& logger_data ); ~Logger_data( void ); Logger_data& operator=( const Logger_data& logger_data ); bool enable; Level level; std::shared_ptr< std::wofstream > logfile; }; typedef std::map< const std::wstring, Logger_data > Logger_data_map; static Logger_data_map m_loggers_data;
Cette instance, grâce au nom de son groupe spécifié par l'utilisateur, va retrouver à sa création ses paramètres dans la partie statique du logger. La fonction log() sert, à partir de cette instance, à récupérer l'ostringstream (temporaire lui aussi) du Logger que j'ai créé, et aussi à spécifier le niveau de logging voulu. Comme je l'ai dit, à la fin de la ligne, cette instance est automatiquement détruite, et c'est donc le destructeur qui s'occupera de logger à proprement parler, mais aussi de flusher mes streams.
Code : Sélectionner tout - Visualiser dans une fenêtre à part rwl::Logger( L"RWL" ).log( rwl::Logger::Level::debug ) << L"Coucou, tu veux voir mon logger?" << std::endl;
En résumé, je flush bel et bien mes données à chaque ligne, et donc si j'ai bien compris ce que ça signifiait, je n'écris pas en RAM. J'ai piqué l'idée de l'instance temporaire à ce monsieur : http://www.drdobbs.com/cpp/logging-in-c/201804215
1) Je pensais que c'était mieux qu'une globale parce que c'était privé dans une classe. Donc en résumé, pour que ça soit propre, il faudrait que je convertisse la partie statique de mon logger en Singleton? Et c'est mieux parce que comme ça c'est moi qui contrôle le moment de son instanciation? (oui je patine un peu, désolé )1)Tu n'as pas de globale, mais tu as une statique de classe (qui est une globale aussi ; elle est stockée au même endroit dans la RAM, et est initialisée au même moment). Je sais qu'on t'a dit que c'est mieux, et dans un sens, c'est vrai. Mais dans ce cas précis, ça ne change strictement rien.
2) Ensuite, même en considérant que ton logger protège l'accès à la map sous-jacente, que se passe-t-il lorsque deux thread essaient d'écrire dans le même fichier de log ?
3)Un dernier problème : le log étant identifié par une valeur qui n'est connue qu'au runtime, tu as une possibilité d'erreur supplémentaire : si tu t'es trompé dans la valeur l'identifiant, le problème ne peut être décelé qu'au runtime, pas à la compilation.
2) Ah oui, je suis bien d'accord. En fait je n'ai absolument pas pensé à cette problématique car je n'ai pas encore besoin d'utiliser plusieurs threads et qu'en plus de ça je ne sais pas vraiment m'en servir. Mais je devrais m'y mettre très prochainement, donc je vais être forcé de résoudre ce problème, je verrais à ce moment là.
3) Là par contre je n'ai pas compris ce que tu veux dire. Tu parles de mes groupes? J'avais considéré que c'était à la charge de l'utilisateur de spécifier correctement le groupe voulu, puisqu'au pire, si il se plante, ça loggera quand même, mais juste pas dans le bon groupe.
En fait je l'ai complètement survolé, son implémentation étant un peu trop costaude pour moi. La seule chose que j'ai reprise (en fait copié collé serait plus juste), c'est leurs différents niveaux de logging car ils sont très pertinents.Si tu avais vraiement ete inspire par Boost.Log
Partager