Discussion :

[Nekkro]

Bonjour,

J'ai une structure définie comme ceci :

Code :

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

1
2
3
4
5
struct X
{
    std::ofstream file;
    // ...
};

et une classe Y qui stocke des X :

Code :

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

1
2
3
4
5
class Y
{
    private:
        std::unordered_map<std::string, X> data;
};

Mon soucis, c'est que lorsque je veux créer un nouveau X et le ranger dans data, le compilateur m'en empêche :

error: use of deleted function 'X& X::operator=(const X&)'
note: 'X& X::operator=(const X&)' is implicitly deleted because the default definition would be ill-formed:
error: use of deleted function 'std::basic_ofstream<char>& std::basic_ofstream<char>::operator=(const std::basic_ofstream<char>&)'

Comment puis-je m'en sortir ? J'aimerais pouvoir faire ceci :

Code :

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

1
2
Y y;
y.add_X ("ID", X(/* ... */));

Comment puis-je le créer sans copie ? (j'utilise un mingw64 avec gcc 4.7).

[Ekleog]

Il faudra utiliser un pointeur quelque part. Soit pour stocker le fstream dans X, soit pour stocker le X dans la map.

[JolyLoic]

Ton gcc gère la move semantic. Il est impossible de copier un fstream, mais il est possible de le déplacer.

Déjà pour activer les nouveautés du C++11, il faut compiler avec le flag -std=c++11

Ensuite, il faut que ton objet contenant un flux soit lui même déplaçable. Et finalement, pour mettre dans ta map, que tu ne génère pas de copie, mais un déplacement.

Peut-être que si tu montre un peu plus de code il sera possible de voir où ça coince ?

[Nekkro]

Je compile déjà en C++11, pour pouvoir utiliser les unordered_map sans boost.

Voici la classe X (partiellement) :

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
struct LogLevel
{
	LogLevel (const std::string & file_name, const std::string & msg_header = "[TRACE]", const std::string & date_format = "[%d/%m/%y - %H:%M]"):
		file (file_name, std::ios::app), header (msg_header), file_name (file_name), date_format (date_format) {}
 
	~LogLevel () { if (file.is_open()) file.close(); }
 
	std::ofstream file;
	std::string   header,
	              file_name,
	              date_format;
};

Et la classe Y (partiellement) :

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
class Logger
{
	public: // -- CONSTRUCTORS
		Logger () = default;
		Logger (std::initializer_list <LogLevel> levels);
 
	public: // -- DATA MANAGEMENT
		void register_level (const std::unordered_map<std::string, LogLevel> & levels)
		{
			for (auto it = levels.begin() ; it != levels.end() ; ++it)
				this->levels[it->first] = LogLevel (it->second.file_name, it->second.header, it->second.date_format); // Problème ici
		}
 
	private: // -- DATA
		std::unordered_map <std::string, LogLevel> levels;
};

J'aimerais pouvoir faire ça au final :

Code :

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

1
2
3
4
logger.register_level (
	{"UN_NIVEAU", LogLevel ("./logs/")},
	{"UN_AUTRE", LogLevel ("./logs/other/")}
);

J'ai pensé utiliser un emplace, mais j'ai des tas d'autres erreurs.

[JolyLoic]

Je n'ai pas trop l'habitude de la move semantic, devant compiler sur des antiquités, mais je dirais à vue de nez :

- Le destructeur dans LogLevel empêche la génération automatique d'un mode constructor de déplacement ou d'un opérateur= par déplacement. Donc soit tu supprimes ce destructeur (qui de toute façon n'a pas l'air utile, a moins que le vrai code fasse d'autres choses), soit tu déclares et défini ces deux opérations (éventuellement avec du =default, pour ne pas avoir à les implémenter). Ta classe est désormais movable

- Comme dans ta ligne this->levels[it->first] = LogLevel (it->second.file_name, it->second.header, it->second.date_format);, ce que tu ajoutes dans ta map est un temporaire, le compilateur devrait choisir la version qui déplace, donc ton code devrait compiler.

- Par contre, avec ce code, tu te retrouves à ouvrir deux fois le fichier de log simultanément, la seconde ouverture va probablement échouer, et donc le flux être dans un état d'erreur, et c'est celle là qui importe... Il serait donc probablement préférable de modifier ta fonction pour qu'elle prenne tes données par r-value ref aussi, prenant alors possession de celles ci, et t'obligeant lors de l'appel à lui passer des données temporaires qui ne seront plus utilisées après.

[Nekkro]

Je te remercie de tes réponses, cependant :

- La mise en commentaire de mon constructeur ne change strictement rien. Par ailleurs, j'ai lu à plusieurs reprises que le destructeur d'un of/if/fstream ne fermait pas le fichier, et que donc il fallait le faire manuellement.
J'ai donc ajouté les deux operations en mode = default; mais elles sont implicitement deleted par mon compilateur car il semblerait que mon ofstream n'ait pas de sémantique de déplacement.

- A cette ligne précise, mon compilateur persiste à vouloir utiliser l'operator= (const ofstream &) plus que la version move, je suppose donc qu'elle n'existe pas.

- De quelle façon puis-je arriver à ce résultat ? J'ai besoin de passer par le constructeur par initializer_list de mon unordered_map pour faire ce dont j'ai besoin, donc je ne peux pas me passer de celle-ci.

[Arzar]

Malheureusement, à en croire la page GCC C++0x Library Features item 27.9

27.9 - File-based streams - Partial - Missing move and swap operations

Du coup impossible de déplacer des fstream avec gcc 4.7...
C'est quand même fort de café quand on sait que la move semantic et les rvalue reference sont implémentés dans le compilateur depuis 2008 !! (gcc 4.3)

[koala01]

Je te remercie de tes réponses, cependant :

- La mise en commentaire de mon constructeur ne change strictement rien. Par ailleurs, j'ai lu à plusieurs reprises que le destructeur d'un of/if/fstream ne fermait pas le fichier, et que donc il fallait le faire manuellement.

Non, un flux est automatiquement fermé quand il est détruit, et automatiquement détruit quand on quitte la portée dans laquelle la variable correspondante est déclarée

Ainsi, l'accolade servant de portée, on peut parfaitement écrire un code 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
17
18
 
int main()
{
    cons char * str="fichier.txt";
    {
        std::ofstream file(str);
        // pas besoin de file.open() car le constructeur utilisé ouvre 
        // d'office le fichier
        // succession d'écritures
    }
    {
        std::ifstream file(str);
        // pas besoin de file.open() car le constructeur utilisé ouvre 
        // d'office le fichier
        // succession de lectures
    }
    return 0;
}

qui va compiler et fonctionner sans le moindre problème

Ce code, bien que simple est riche en enseignements, car, il nous permet de constater

1. Que bien que j'utilise (intentionnellement pour l'exemple ) deux fois le même nom de variable (file, en l'occurrence), le compilateur ne se plaint absolument pas d'une déclaration multiple de variable, car la variable file n'existe chaque fois qu'entre sa déclaration et l'accolade fermante dans laquelle elle a été déclarée
2. Que le(s) constructeurs de fstream prenant en premier paramètre le nom de fichier ouvrent automatiquement le fichier dans le mode correspondant (lecture pour ifstream, écriture pour ofstream)
3. Que, si l'on ne précise pas d'argument supplémentaire, les flux sont ouverts en "mode texte" (par opposition au mode "binaire")
4. Que dés que le flux est détruit, le fichier est correctement fermé (autrement, l'exécution planterait lorsqu'on essayerait d'ouvrir le ofstream)

J'ai donc ajouté les deux operations en mode = default; mais elles sont implicitement deleted par mon compilateur car il semblerait que mon ofstream n'ait pas de sémantique de déplacement.

- A cette ligne précise, mon compilateur persiste à vouloir utiliser l'operator= (const ofstream &) plus que la version move, je suppose donc qu'elle n'existe pas.

cf la réponse de Arzar, je ne connais pas non plus bien la move semantic ...

Vérifies peut etre du coté de Gcc 4.8.xx

- De quelle façon puis-je arriver à ce résultat ? J'ai besoin de passer par le constructeur par initializer_list de mon unordered_map pour faire ce dont j'ai besoin, donc je ne peux pas me passer de celle-ci.

Restes seulement bien dans un environnement utilisant C++11

Par contre, travailles peut etre "à l'ancienne" en ce qui concerne les flux ! :
Depuis toujours (donc, depuis bien avant l'introduction de la sémantique de mouvement les flux ne sont pas copiables.

l'une des solutions envisageables est donc d'ouvrir le fichier (en mode "append" !! ) "à la demande" (comprends : chaque fois que l'on a besoin d'y écrire quelque chose), et de le refermer une fois que l'on a fini d'y écrire ce que l'on devait y écrire

Pour y arriver, il "suffit" que ta classe maintienne non pas le flux mais bien le nom du fichier à utiliser, ce qui fait que ta classe X pourrait ressembler à quelque chose comme

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
class X
{
    public:
        X(std::string const & filename):filename_(filename){}
        /* transmettons un "range" de chaines de caractères correspondant
         * aux informations à écrire dans le fichier
         */
        template <iterator>
        void write( iterator begin, iterator end)
        {
            std::ofstream ofs(filename_,std::app)
            while(begin != end)
            {
                ofs << *it<<" ";
            }
            ofs<<std::endl;
        }
    private:
        std::string filename_;
};

L'autre solution consiste à garder le flux ouvert en permanence et à en éviter la copie.

Pour éviter la copie d'une variable lorsqu'on la transmet à une fonction, on utilise, tout simplement, une référence sur cet objet car une référence peut parfaitement etre membre d'une classe, pour autant qu'elle soit définie dans le constructeur

Il y a "à boire et à manger" dans les deux solutions, car la première risque de "plomber" les performances chaque fois que tu voudra logger quelque chose (il faut compter le temps d'ouverture et de fermeture du fichier pour chaque écriture ), mais elle permettra, en échange, de pouvoir lire le fichier log alors que l'application tourne (ce qui peut, dans certaines situations, etre intéressant quand meme )

La deuxième solution éviterait les ouvertures et fermetures continuelles de fichier (et ferait donc potentiellement gagner en performance, au prix d'une utilisation accrue de la mémoire) , mais, par contre, empêcherait l'ouverture du fichier par une application externe tant que l'application tourne.

de plus, il n'y a que deux solutions pour rendre la classe Logger responsable de la durée de vie des flux:

Soit tu crées un membre de type std::ofstream par flux envisagé (et donc par LoggerLevel), mais tu contreviens alors à l'OCP (Open / Close Principle) dans le sens où tu devras rajouter "manuellement" le membre dans la classe Logger

Soit, tu utilises l'allocation dynamique de la mémoire pour créer les flux à la demande et tu maintiens une liste de pointeurs sur ces flux au niveau de ta classe Logger (quitte à passer "ce qui est pointé par le pointeur" au constructeur de ta classe LogLevel ) mais il faut alors s'assurer que la mémoire allouée aux flux sera libérée en temps et en heure.

Un petit tour du coté des smart_pointer fournis par C++11 devrait, en cela, te faciliter un peu la vie

[Ekleog]

Malheureusement, à en croire la page GCC C++0x Library Features item 27.9

Du coup impossible de déplacer des fstream avec gcc 4.7...
C'est quand même fort de café quand on sait que la move semantic et les rvalue reference sont implémentés dans le compilateur depuis 2008 !! (gcc 4.3)

Tu te proposes pour faire un patch ?

[Nekkro]

Merci beaucoup pour cette réponse complète, Koala01 !

Il va falloir que je revoie absolument toute ma gestion parce qu'apparemment mon compilateur ne reconnait pas cette syntaxe :

Code :

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

1
2
3
4
logger.register_level (
	{"UN_NIVEAU", LogLevel<> ("./logs/log.txt")},
	{"UN_AUTRE", LogLevel<LogPolicy::FOLDER_POLICY> ("./logs/other/")}
);

Comme étant celle appelant le constructeur implicite de la unordered_map avec initializer_list ... :

error: no matching function for call to 'Logger::register_level(<brace-enclosed initializer list>, <brace-enclosed initializer list>)'

Donc je sais pas trop comment je vais me débrouiller pour pouvoir ajouter d'un coup plusieurs niveaux (c'est un peu comme devoir faire plein de push_back pour initialiser un vector en pre-C++11 ... :/)

J'ai essayé de me rabattre sur l'ajout d'un élément, en virant temporairement le fichier de LogLevel pour me simplifier la tâche, mais même ça, le compilo veut pas =/

Code :

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

1
2
3
4
5
6
7
8
void register_level (const std::string & name, const LogLevel & level)
{
	levels[name] = level;
}
 
// en faisant ça :
 
logger.register_level ("ALERT", LogLevel ("./logs/log.txt"));

error: no matching function for call to 'LogLevel::LogLevel()'
note: candidates are:
note: LogLevel::LogLevel(const string&, const string&, const string&)
note: LogLevel::LogLevel(const LogLevel&)

Pourquoi diable me demande-t-il le constructeur par défaut alors que je passe le LogLevel par référence constante ? (Évidemment si je rajoute le constructeur par défaut, tout va bien, mais j'aimerais bien savoir pourquoi !)

[JolyLoic]

Merci beaucoup pour cette réponse complète, Koala01 !

Il va falloir que je revoie absolument toute ma gestion parce qu'apparemment mon compilateur ne reconnait pas cette syntaxe :

Code :

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

1
2
3
4
logger.register_level (
	{"UN_NIVEAU", LogLevel<> ("./logs/log.txt")},
	{"UN_AUTRE", LogLevel<LogPolicy::FOLDER_POLICY> ("./logs/other/")}
);

Comme étant celle appelant le constructeur implicite de la unordered_map avec initializer_list ...

Il ta manque un niveau d'accolades...

Code :

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

1
2
3
4
5
logger.register_level ({
	{"UN_NIVEAU", LogLevel<> ("./logs/log.txt")},
	{"UN_AUTRE", LogLevel<LogPolicy::FOLDER_POLICY> ("./logs/other/")}
             }
);

Sinon, si les flux ne sont pas movable avec ton compilateur, il va alors de falloir passer par une variable membre de type unique_ptr<ofstream> dans LogLevel, soit avec un unique_ptr<LogLevel> dans ta map.

[Nekkro]

Comble :

Mes unique_ptr ne sont ni copiables ni déplaçable apparemment.

Donc je vais devoir m'en tenir à des ofstream* ...

[Ekleog]

unique_ptr est parfaitement déplaçable, hein.

[Nekkro]

Dans ce cas je comprend pas trop pourquoi mon compilateur refuse de compiler lorsque je l'utilise :/

Autre problème tant que j'y suis :

j'ai rajouté cette ligne dans LogLevel :

Code :

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

constexpr static const char * INFO = "INFO";

Et impossible de faire :

Code :

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

cout << LogLevel::INFO << endl;

car j'ai une undefined reference dessus ...

Une idée ?

[Ekleog]

constexpr const ?

Il me semble que la syntaxe devrait plutôt être static constexpr char * INFO = "INFO";. Non ?

Après, quand j'ai utilisé gcc4.6 (pas testé 4.7), j'avais aussi des undefined reference's, mais je ne me souviens plus si j'avais réussi à résoudre le problème ou pas.

Après un peu de duckduckgo'ing (oui, c'est moins beau que googling, mais aussi efficace) : http://stackoverflow.com/questions/8...constexpr-char (premier résultat)

[Nekkro]

En effet ça marche si l'on redéfinit dans le .cpp.

Pour simplifier mes tests, j'ai créé une structure qui ne contient qu'un unique qunique_ptr<ofstream>.

Mais quoi que j'essaye, je n'arrive pas à rentrer une instance de Test dans ma map, même en passant une simple référence.

Donc mon implémentation de mingw64 n'implémente pas encore le move pour mes unique_ptr, je ne vois que ça.

EDIT : En allant chercher dans les fichiers du compilo, je trouve bien un constructeur par déplacement et un operateur de déplacement, mais apparemment je ne sais pas comment faire pour l'utiliser ^^

Autre problème : quand j'utilise const LogLevel & comme paramètre d'une fonction, j'ai une copie qui est faite. J'ai des problèmes à la destruction de la copie et du LogLevel final, donc pour éviter les copies en attendant de pouvoir utiliser le déplacement, je suis obligé de passer par une référence, non constante.

EDIT 2 : Et encore que non en fait, car lorsque la variable qui a servit à être passée par référence est détruite, sa copie dans la map de Logger est aussi détruite, et hop recrash.
Je suis obligé de faire des pointeurs sur LogLevel si je ne veux pas avoir de crash avec l'ofstream et si je ne veux aucune copie où je n'en veux pas.
Bref c'est vraiment trop compliqué pour être honnête.

[koala01]

Ben, il te reste donc l'autre solution : n'avoir, dans le loglevel, que le nom du fichier pour l'ouvrir et le fermer à chaque écriture

De toutes manières, c'est peut etre le plus intelligent, pour un fichier log, car cela te permettra de lire le fichier alors que l'application fonctionne encore et que l'on peut, malgré tout, considérer que tu ne fera pas des écritures ne serait-ce que toutes les secondes

[koala01]

Ceci dit, j'aurais tendance à dire que, entre Logger et LogLevel, celui qui va s'occuper d'écrire dans le fichier (et qui en a donc besoin, que ce soit sur une instance de ofstream ou au travers du nom du fichier à ouvrir), c'est Logger, car tu ne vas sans doute pas créer un fichier différent par niveau de log, mais bel et bien placer tous les élément à logger dans un seul et même fichier.

LogLevel n'est là "que" pour te permettre de formater l'information (et surtout pour te permettre de mettre le bon terme au début de la ligne)

[Ekleog]

Lorsque tu prends un paramètre comme const LogLevel &, il ne devrait pas y avoir de copie. Serait-il possible d'avoir un code minimal reproduisant le bug ?

koala01: Je pense qu'il vaut clairement mieux garder le fichier ouvert. Surtout que -- sous linux en tout cas -- avoir un fichier ouvert par deux processes en même temps n'est pas un souci. D'ailleurs, pourquoi serait-ce impossible quand tail -f existe ?

Par ailleurs, il semble que le compilateur supporte le move d'unique_ptr. Donc c'est probablement une erreur d'utilisation, ce qui est possible à résoudre !

[koala01]

Lorsque tu prends un paramètre comme const LogLevel &, il ne devrait pas y avoir de copie. Serait-il possible d'avoir un code minimal reproduisant le bug ?

koala01: Je pense qu'il vaut clairement mieux garder le fichier ouvert. Surtout que -- sous linux en tout cas -- avoir un fichier ouvert par deux processes en même temps n'est pas un souci. D'ailleurs, pourquoi serait-ce impossible quand tail -f existe ?

Comme tu le dis si bien : sous linux, il n'y a pas de problème... et sous linux, tail -f existe en effet, mais pas sous windows

Sous windows, Il existe bien quelques applications qui vont, en réalité, créer une copie temporaire du fichier qu'elles utiliseront tant qu'elles doivent y avoir acces (pour éviter de garder le fichier verrouillé trop longtemps, quitte à comparer de temps en temps (assez régulièrement, en fait ) le fichier d'origine (ou en fait sans doute son time code ) avec le fichier temporaire qu'elles utilise, mais, si tu ne fais pas de la sorte, le fiichier reste verrouillé tant que l'application qui l'utilise ne l''aura pas fermé

Ceci dit, il y a très facilement moyen de faire en sorte que la politique d'ouverture / fermeture du fichier log diffère en fonction du niveau de logging souhaité lors de l'exécution

Il peut, par exemple, en effet etre utile de logger toutes les informations en débug, histoire de pouvoir déterminer un éventuel scenario, alors que, d'un autre coté, le log de production peut "se limiter" aux informations de sécurité et de criticité importante

Il ne serait pas particulièrement difficile (pimpl idiome aidant ) de faire en sorte que le logging en debug garde le fichier ouvert et que le logging en prod ne l'ouvre que "à la demande"

Par ailleurs, il semble que le compilateur supporte le move d'unique_ptr. Donc c'est probablement une erreur d'utilisation, ce qui est possible à résoudre !

Ca, c'est une possibilité qui nécessite d'être creusée, mais qui n'est certainement pas sans fondement