Discussion :

[Aleph69]

Bonjour,

j'essaie de lancer et capturer la bonne exception en enveloppant un code C qui appelle fopen.

Concrètement, j'ai une classe pour gérer un type de fichier, disons File (admettant pour donnée membre un pointeur sur FILE nommé content_) dont le constructeur s'écrit

Code :

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

1
2
3
4
5
6
  File::File(std::string const & filename)
  : content_(fopen(filename.c_str(), "r"))
  {
    if(!content_)
      throw new std::ios_base::failure("Unable to open the specified file");
  }

Je fais appel à ce constructeur pour un fichier n'existant pas, en tentant de capturer l'exception que je lance dans le constructeur :

Code :

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

1
2
3
4
5
6
7
8
  try
  {
    File * does_not_exist = new File("Does not exist");
  }
  catch(std::ios_base::failue const & e)
  {
    std::cout << e.what() << std::endl;
  }

Malheureusement, je n'attrape pas l'exception avec cette ligne de code.
En revanche, si je remplace mon appel à catch par catch(...), une exception est bien attrapée.

Est-ce que vous sauriez pourquoi?

EDIT : cela ne fonctionne pas non plus si j'essaie d'attraper une instance de std::exception.

Merci!

[Aleph69]

Comme prévu, c'était une erreur de ma part.
Le problème venait du lancer d'exception, qu'il fallait faire par valeur.

[JolyLoic]

Toujours lancer une exception par valeur, et l'attraper par référence.

Sinon, tu es conscient que dans ton code, en cas d'exception, tu as non seulement une fuite mémoire, mais une fuite de handle de fichier (pour peu qu'une exception soit lancée après une ouverture ayant réussi) ?

[Aleph69]

Salut Loïc,

merci pour ta réponse.
Je veux bien que tu détailles un peu ta réponse.

Mon raisonnement naïf est le suivant :
- fopen ne lève pas d'exception;
- je lève une exception si et seulement si content_ vaut NULL, c'est-à-dire si et seulement aucun fichier n'a été ouvert : donc aucun fichier n'a à être fermé;
- si je ne lève pas d'exception, content_ ne vaut pas NULL et le fichier sera fermé dans le destructeur de FIle (RAII).

Du coup, je ne vois ni la fuite mémoire, ni la fuite de handle de fichier.
Un peu d'aide est donc la bienvenue!

Et encore merci!

[prgasp77]

EDIT : Attention, j'ai écris ce message alors que j'avais 39°C de fièvre. Comme relevé ci-après, certains passages sont faux.

Bonjour,

la ligne File * does_not_exist = new File("Does not exist"); est très riche, et sous son apparence simple procède à plusieurs opérations :

1. new réserve sizeof(File) octets ;
2. un objet File est construit à cet emplacement mémoire (un std::string est construit puis détruit dans le processus) ;
3. l'adresse de la plage réservée est affectée à does_not_exist.

(N.B. : 2 et 3 peuvent être inversés / parallélisés).

Ainsi, si 2 lève un exception, la plage mémoire réservée en 1 n'est jamais libérée.

[Aleph69]

Salut!

Merci pour ta réponse.
Ce bout de code est juste un exemple jouet!
Si c'est lui qui pose problème, alors pas d'inquiétude!

[prgasp77]

Après vérification, std::string::string, std::string:c_str et std::string::~string ne lèvent pas de std::ios_base::failure donc tu sembles exempt de fuite de handle de fichier ; ceci dit je parie que tu n'avais pas fait cette vérification. En général, je conseille d'éviter toute opération qui pourrait lever une exception dans un constructeur ou un destructeur. Plutôt que d'ouvrir le fichier dans le constructeur, contente-toi d'acquérir les ressources qui te sont passées et retarde l'appel de fopen au dernier moment.

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
class File
{
    std::string const _filename;
    ::FILE* _fhandle;
public:
    File(std::string const& filename) : _filename(filename) /* std::string copy since filename may be a temporary */ {}
    void open()
    {
        _fhandle = ::fopen(_filename.c_str(), "r");
        if (NULL == _fhandle) {
            throw std::ios_base::failure("Unable to open the specified file");
        }
    }
    /* ... */
};

De la sorte, plus de new dans un try catch .
Cdlt,

[Aleph69]

Tu as raison : je n'avais pas vérifié!
Je t'avoue que je préfère respecter le RAII, mais je garde en tête de prendre garde aux exceptions qui pourraient être lancées à mon insu!
Merci beaucoup!

[Luc Hermitte]

???
Pourquoi voulez-vous retarder l'ouverture ? Vous affaiblissez l'invariant de l'objet en procédant de la sorte. Avant de s'en servir, il faudra à chaque fois tester dynamiquement (ou à coups d'asserts) si l'utilisateur n'a pas été boulet et chercherait à accéder au fichier sans l'avoir ouvert.

Lever des exceptions dans des constructeurs, ça marche très bien (à la limite on pourrait pinailler que la vérification des inputs doit être faite en amont avant la construction).

Le premier principe du RAII, c'est que la ressource est acquise (ou associée à la capsule) dans le constructeur -- fopen est justement une acquisition de ressource. C'est ce qui lui vaut son nom. Le second principe, c'est la destruction déterministe, c'est ce qui lui vaut sa renommée.

Les problèmes du code sont:
- le new File (ce qui est toujours louche). new est devenu un code smell en C++
- le fait que si il y a des choses dans le constructeur après le fopen, il peut y avoir une fuite de ressource fichier -- n'en ayant pas vu, je ne vois pas ce qui a fait réagir Loic

unique_ptr peut être notre ami ici -- et en plus, il règle les problèmes de copie

Code :

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

1
2
3
4
5
6
7
8
9
10
11
12
// code vite pondu et non testé
struct File {
    File(std::string const& s)
    : m_name(s)
    , m_file{fopen(s.c_str(), "r"), [](FILE* p){ if(p) fclose(p);} } // pas sûr pour la syntaxe ici
    {
        if (!m_file) throw .....
    }
private:
    std::string m_name;
    std::unique_ptr<FILE> m_file;
};

@prgasp77
Dans "new T{args}", si le constructeur lève une exception, la mémoire sera libérée. Encore heureux que le standard nous le garantisse. Il nous garantit même que tous les membres construits seront détruits, et que le destructeur ne sera pas appelé, ce qui est normal.
Ici, l'OP lève son exception si fopen renvoie nullptr, il ne faut surtout pas appeler fclose dans ce cas là. Son début de code était bon, même si très perfectible à cause du non respect de l'invariant "tout fichier ouvert sera détruit une et une seule fois" vu que la copie n'avait pas été blindée.

[JolyLoic]

- le fait que si il y a des choses dans le constructeur après le fopen, il peut y avoir une fuite de ressource fichier -- n'en ayant pas vu, je ne vois pas ce qui a fait réagir Loic

C'était ces point là :
- S'il y a une exception dans le constructeur après un fopen réussi (pas possible ici, mais si le code évolue...)
- S'il y a une exception dans le try après un new qui aurait réussi (idem).

Certes, il n'y a pas de problème avéré dans le code, mais il y en a des potentiels, ce n'est pas très robuste, et c'est pour ça que je demandais à l'auteur s'il en était conscient. Si oui, pas de soucis, c'est juste un extrait de code, incomplet par essence, si non, c'est gênant.

Avec la proposition de Luc et l'utilisation directe d'une variable plutôt que d'un pointeur :

Code :

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

1
2
3
4
5
6
7
8
  try
  {
    auto does_not_exist = File{"Does not exist"};
  }
  catch(std::ios_base::failue const & e)
  {
    std::cout << e.what() << std::endl;
  }

le code me semble bien plus sûr.

[prgasp77]

Dans "new T{args}", si le constructeur lève une exception, la mémoire sera libérée. Encore heureux que le standard nous le garantisse. Il nous garantit même que tous les membres construits seront détruits, et que le destructeur ne sera pas appelé, ce qui est normal.
Ici, l'OP lève son exception si fopen renvoie nullptr, il ne faut surtout pas appeler fclose dans ce cas là. Son début de code était bon, même si très perfectible à cause du non respect de l'invariant "tout fichier ouvert sera détruit une et une seule fois" vu que la copie n'avait pas été blindée.

Oui bien entendu. J'ai écris ce message alors que j'étais malade, ma capacité cognitive était bien plus impactée que je ne le pensais.

Pourquoi voulez-vous retarder l'ouverture ? [...] Le premier principe du RAII, c'est que la ressource est acquise (ou associée à la capsule) dans le constructeur

J'ai pris l'habitude --- et cela paye jusqu'à présent --- de laisser le soin à l'appelant la création de la ressource (ici, appeler ::fopen) et de la transmettre au constructeur (qui ici prendrait un handle de fichier donc) OU* de transmettre au constructeur les données nécessaires à l'acquisition d'une ressource, acquisition qui sera faite en dehors du constructeur (ce n'est plus à mon sens du RAII à proprement parler). Dans les deux cas, cela me donne des constructeurs quasi-noexcept et me permet de construire mes objets où je veux (pourquoi pas en variable globale), de la manière que je veux (statique, dynamique) d'une manière très analogue. Ce n'est pas la solution, une parmi tant d'autres.

*je préfère le premier des deux paradigmes.

[Aleph69]

Bonsoir,

tout d'abord, merci à tous pour vos commentaires.

new est devenu un code smell en C++

Cela m'intéresse d'en savoir plus.
Est-ce que tu as des sources sur le sujet?

Je comprendrais bien

new en dehors d'un constructeur est devenu un code smell en C++

mais faut-il comprendre autre chose dans ta phrase?

[Médinoc]

En gros, depuis C++14 on utilise make_unique() à la place, qui retourne directement un unique_ptr.

[Luc Hermitte]

delete hors destructeur était pour moi déjà un code smell (des exceptions sont possibles dans les opérateurs d'affectation)

Et comme l'a dit medinoc, depuis make_unique, et tous les autres make_xxx, new devient aussi un code smell.

[Aleph69]

Je ne connaissais pas les make functions.
Effectivement, je comprends que new soit maintenant considéré comme un code smell.
Merci Médinoc et Luc!