Utilisation valide de goto pour la gestion des erreurs en C?

92

Cette question est en fait le résultat d'une discussion intéressante sur programmation.reddit.com il y a quelque temps. Cela se résume essentiellement au code suivant:

int foo(int bar)
{
    int return_value = 0;
    if (!do_something( bar )) {
        goto error_1;
    }
    if (!init_stuff( bar )) {
        goto error_2;
    }
    if (!prepare_stuff( bar )) {
        goto error_3;
    }
    return_value = do_the_thing( bar );
error_3:
    cleanup_3();
error_2:
    cleanup_2();
error_1:
    cleanup_1();
    return return_value;
}

L'utilisation de gotohere semble être la meilleure voie à suivre, aboutissant au code le plus propre et le plus efficace de toutes les possibilités, ou du moins il me semble. Citant Steve McConnell dans Code Complete :

Le goto est utile dans une routine qui alloue des ressources, effectue des opérations sur ces ressources, puis désalloue les ressources. Avec un goto, vous pouvez nettoyer dans une section du code. Le goto réduit la probabilité que vous oubliez de désallouer les ressources à chaque endroit où vous détectez une erreur.

Une autre prise en charge de cette approche provient du livre Linux Device Drivers , dans cette section .

Qu'est-ce que tu penses? Ce cas est-il une utilisation valide pour gotoen C? Préférez-vous d'autres méthodes, qui produisent du code plus compliqué et / ou moins efficace, mais à éviter goto?

Eli Bendersky
la source
@Eli: Pourquoi ne supprimez-vous pas les balises et placez-vous la fonction (cleanup_3 ();) entre parenthèses de if?
@Akito: que voulez-vous dire? Pourriez-vous publier votre suggestion sous forme de réponse avec un exemple de code complet?
Eli Bendersky
@EliBendersky: S'il vous plaît voir ma réponse.
L'une des choses que je détestais le plus de Visual Basic (VBS et VB6 inclus) était le on error goto errorsystème de gestion des erreurs :)
Manu343726

Réponses:

61

FWIF, je trouve que l'idiome de gestion des erreurs que vous avez donné dans l'exemple de la question est plus lisible et plus facile à comprendre que l'une des alternatives données dans les réponses jusqu'à présent. Bien que ce gotosoit une mauvaise idée en général, elle peut être utile pour la gestion des erreurs lorsqu'elle est effectuée de manière simple et uniforme. Dans cette situation, même s'il s'agit d'un goto, il est utilisé de manière bien définie et plus ou moins structurée.

Michael Burr
la source
1
Ne peut-il pas simplement supprimer ces balises et mettre les fonctions directement dans if? Ne sera-ce pas plus lisible?
8
@StartupCrazy, je sais que cela a des années, mais pour la validité des messages sur ce site, je ferai remarquer que non, il ne peut pas. S'il obtient une erreur à goto error3 dans son code, il exécutera le nettoyage 1 2 et 3, dans votre solution suggérée, il n'exécutera que le nettoyage 3. Il pourrait imbriquer des choses, mais ce serait juste la flèche anti-modèle, ce que les programmeurs devraient éviter .
gbtimmon le
17

En règle générale, éviter goto est une bonne idée, mais les abus qui prévalaient lorsque Dijkstra a écrit pour la première fois `` GOTO considéré comme nuisible '' ne traversent même pas l'esprit de la plupart des gens comme une option ces jours-ci.

Ce que vous décrivez est une solution généralisable au problème de gestion des erreurs - cela me convient tant qu'il est soigneusement utilisé.

Votre exemple particulier peut être simplifié comme suit (étape 1):

int foo(int bar)
{
    int return_value = 0;
    if (!do_something(bar)) {
        goto error_1;
    }
    if (!init_stuff(bar)) {
        goto error_2;
    }
    if (prepare_stuff(bar))
    {
        return_value = do_the_thing(bar);
        cleanup_3();
    }
error_2:
    cleanup_2();
error_1:
    cleanup_1();
    return return_value;
}

Poursuivre le processus:

int foo(int bar)
{
    int return_value = 0;
    if (do_something(bar))
    {   
        if (init_stuff(bar))
        {
            if (prepare_stuff(bar))
            {
                return_value = do_the_thing(bar);
                cleanup_3();
            }
            cleanup_2();
        }
        cleanup_1();
    }
    return return_value;
}

C'est, je crois, équivalent au code original. Cela semble particulièrement propre car le code d'origine était lui-même très propre et bien organisé. Souvent, les fragments de code ne sont pas aussi ordonnés que cela (bien que j'accepte un argument selon lequel ils devraient l'être); par exemple, il y a souvent plus d'états à passer aux routines d'initialisation (setup) qu'indiqué, et donc plus d'états à passer aux routines de nettoyage également.

Jonathan Leffler
la source
23
Oui, la solution imbriquée est l'une des alternatives viables. Malheureusement, il devient moins viable à mesure que le niveau de nidification s'approfondit.
Eli Bendersky
4
@eliben: D'accord - mais l'imbrication plus profonde pourrait (est probablement) une indication que vous devez introduire plus de fonctions, ou demander aux étapes de préparation d'en faire plus, ou de refactoriser votre code. Je pourrais affirmer que chacune des fonctions de préparation devrait faire sa configuration, appeler la suivante dans la chaîne et faire son propre nettoyage. Il localise ce travail - vous pourriez même économiser sur les trois fonctions de nettoyage. Cela dépend également, en partie, du fait que l'une des fonctions de configuration ou de nettoyage est utilisée (utilisable) dans une autre séquence d'appel.
Jonathan Leffler
6
Malheureusement, cela ne fonctionne pas avec les boucles - si une erreur se produit à l'intérieur d'une boucle, alors un goto est beaucoup, beaucoup plus propre que l'alternative de définir et de vérifier des indicateurs et des instructions `` break '' (qui ne sont que des gotos intelligemment déguisés).
Adam Rosenfield
1
@Smith, Plus comme conduire sans gilet pare-balles.
strager
6
Je sais que je nécromance ici, mais je trouve ce conseil plutôt médiocre - la flèche anti-motif doit être évitée.
KingRadical
16

Je suis surpris que personne n'ait suggéré cette alternative, donc même si la question existe depuis un certain temps, je vais l'ajouter: une bonne façon de résoudre ce problème est d'utiliser des variables pour garder une trace de l'état actuel. Il s'agit d'une technique qui peut être utilisée, qu'elle soit ou non gotoutilisée pour arriver au code de nettoyage. Comme toute technique de codage, elle a des avantages et des inconvénients et ne conviendra pas à toutes les situations, mais si vous choisissez un style, cela vaut la peine d'être pris en compte - surtout si vous voulez éviter gotosans vous retrouver avec des ifs profondément imbriqués .

L'idée de base est que, pour chaque action de nettoyage qui pourrait être nécessaire, il existe une variable à partir de laquelle nous pouvons dire si le nettoyage doit être effectué ou non.

Je vais d'abord montrer la gotoversion, car elle est plus proche du code de la question d'origine.

int foo(int bar)
{
    int return_value = 0;
    int something_done = 0;
    int stuff_inited = 0;
    int stuff_prepared = 0;


    /*
     * Prepare
     */
    if (do_something(bar)) {
        something_done = 1;
    } else {
        goto cleanup;
    }

    if (init_stuff(bar)) {
        stuff_inited = 1;
    } else {
        goto cleanup;
    }

    if (prepare_stuff(bar)) {
        stufF_prepared = 1;
    } else {
        goto cleanup;
    }

    /*
     * Do the thing
     */
    return_value = do_the_thing(bar);

    /*
     * Clean up
     */
cleanup:
    if (stuff_prepared) {
        unprepare_stuff();
    }

    if (stuff_inited) {
        uninit_stuff();
    }

    if (something_done) {
        undo_something();
    }

    return return_value;
}

Un avantage de ceci par rapport à certaines des autres techniques est que, si l'ordre des fonctions d'initialisation est changé, le nettoyage correct se produira toujours - par exemple, en utilisant la switchméthode décrite dans une autre réponse, si l'ordre d'initialisation change, alors le switchdoit être édité très soigneusement pour éviter d'essayer de nettoyer quelque chose qui n'a pas été réellement initialisé en premier lieu.

Maintenant, certains pourraient argumenter que cette méthode ajoute un grand nombre de variables supplémentaires - et en effet dans ce cas c'est vrai - mais en pratique, une variable existante suit déjà, ou peut être amenée à suivre, l'état requis. Par exemple, si le prepare_stuff()est en fait un appel à malloc(), ou à open(), alors la variable contenant le pointeur ou le descripteur de fichier renvoyé peut être utilisée - par exemple:

int fd = -1;

....

fd = open(...);
if (fd == -1) {
    goto cleanup;
}

...

cleanup:

if (fd != -1) {
    close(fd);
}

Maintenant, si nous suivons en plus l'état de l'erreur avec une variable, nous pouvons éviter gotocomplètement, et toujours nettoyer correctement, sans avoir d'indentation qui devient de plus en plus profonde au fur et à mesure que nous avons besoin d'initialisation:

int foo(int bar)
{
    int return_value = 0;
    int something_done = 0;
    int stuff_inited = 0;
    int stuff_prepared = 0;
    int oksofar = 1;


    /*
     * Prepare
     */
    if (oksofar) {  /* NB This "if" statement is optional (it always executes) but included for consistency */
        if (do_something(bar)) {
            something_done = 1;
        } else {
            oksofar = 0;
        }
    }

    if (oksofar) {
        if (init_stuff(bar)) {
            stuff_inited = 1;
        } else {
            oksofar = 0;
        }
    }

    if (oksofar) {
        if (prepare_stuff(bar)) {
            stuff_prepared = 1;
        } else {
            oksofar = 0;
        }
    }

    /*
     * Do the thing
     */
    if (oksofar) {
        return_value = do_the_thing(bar);
    }

    /*
     * Clean up
     */
    if (stuff_prepared) {
        unprepare_stuff();
    }

    if (stuff_inited) {
        uninit_stuff();
    }

    if (something_done) {
        undo_something();
    }

    return return_value;
}

Encore une fois, il y a des critiques potentielles à ce sujet:

  • Tous ces «si» ne nuisent-ils pas à la performance? Non - car en cas de succès, vous devez quand même faire toutes les vérifications (sinon vous ne vérifiez pas tous les cas d'erreur); et en cas d'échec, la plupart des compilateurs optimiseront la séquence des if (oksofar)vérifications échouées en un seul saut vers le code de nettoyage (GCC le fait certainement) - et dans tous les cas, le cas d'erreur est généralement moins critique pour les performances.
  • N'est-ce pas ajouter encore une autre variable? Dans ce cas oui, mais souvent la return_valuevariable peut être utilisée pour jouer le rôle qui oksofarjoue ici. Si vous structurez vos fonctions pour renvoyer des erreurs de manière cohérente, vous pouvez même éviter la seconde ifdans chaque cas:

    int return_value = 0;
    
    if (!return_value) {
        return_value = do_something(bar);
    }
    
    if (!return_value) {
        return_value = init_stuff(bar);
    }
    
    if (!return_value) {
        return_value = prepare_stuff(bar);
    }
    

    L'un des avantages d'un tel codage est que la cohérence signifie que tout endroit où le programmeur d'origine a oublié de vérifier la valeur de retour ressort comme un pouce endolori, ce qui facilite la recherche de (cette classe de) bogues.

Donc - c'est (encore) un autre style qui peut être utilisé pour résoudre ce problème. Utilisé correctement, il permet un code très propre et cohérent - et comme toute technique, entre de mauvaises mains, il peut finir par produire du code long et déroutant :-)

psmears
la source
2
On dirait que vous êtes en retard à la fête, mais j'aime certainement la réponse!
Linus rejetterait probablement votre code blogs.oracle.com/oswald/entry/is_goto_the_root_of
Fizz
1
@ user3588161: S'il le faisait, c'est sa prérogative - mais je ne suis pas sûr, d'après l'article auquel vous avez lié, que ce soit le cas: notez que dans le style que je décris, (1) les conditions ne s'imbriquent pas arbitrairement profondément, et (2) il n'y a pas de déclarations "si" supplémentaires par rapport à ce dont vous avez besoin de toute façon (en supposant que vous allez vérifier tous les codes de retour).
psmears
Tellement cela au lieu de l'horrible goto et encore pire de la solution anti-flèche!
Le Croissant Paramagnétique
8

Le problème avec le gotomot-clé est généralement mal compris. Ce n'est pas du mal. Vous devez juste être conscient des chemins de contrôle supplémentaires que vous créez avec chaque goto. Il devient difficile de raisonner sur votre code et donc sur sa validité.

FWIW, si vous recherchez des didacticiels developer.apple.com, ils adoptent l'approche goto pour la gestion des erreurs.

Nous n'utilisons pas de gotos. Une importance plus élevée est accordée aux valeurs de retour. La gestion des exceptions se fait via setjmp/longjmp- tout ce que vous pouvez.

dirkgently
la source
8
Bien que j'aie certainement utilisé setjmp / longjmp dans certains cas où il était demandé, je le considérerais encore "pire" que goto (que j'utilise aussi, un peu moins avec réserve, quand il est demandé). Les seules fois où j'utilise setjmp / longjmp sont lorsque soit (1) La cible va tout arrêter d'une manière qui ne sera pas affectée par son état actuel, ou (2) La cible va réinitialiser tout ce qui est contrôlé à l'intérieur le bloc setjmp / longjmp-gardé d'une manière qui est indépendante de son état actuel.
supercat du
4

Il n'y a rien de moralement mal dans la déclaration goto, pas plus qu'il n'y a quelque chose de moralement faux avec les pointeurs (void) *.

Tout dépend de la façon dont vous utilisez l'outil. Dans le cas (trivial) que vous avez présenté, une déclaration de cas peut atteindre la même logique, mais avec plus de surcharge. La vraie question est: "quelle est ma vitesse requise?"

goto est tout simplement rapide, surtout si vous faites attention de vous assurer qu'il se compile en un saut court. Parfait pour les applications où la vitesse est une prime. Pour les autres applications, il est probablement judicieux de prendre la surcharge avec if / else + case pour la maintenabilité.

Rappelez-vous: goto ne tue pas les applications, les développeurs tuent les applications.

MISE À JOUR: Voici l'exemple de cas

int foo(int bar) { 
     int return_value = 0 ; 
     int failure_value = 0 ;

     if (!do_something(bar)) { 
          failure_value = 1; 
      } else if (!init_stuff(bar)) { 
          failure_value = 2; 
      } else if (prepare_stuff(bar)) { 
          return_value = do_the_thing(bar); 
          cleanup_3(); 
      } 

      switch (failure_value) { 
          case 2: cleanup_2(); 
          case 1: cleanup_1(); 
          default: break ; 
      } 
} 
webmarc
la source
1
Pourriez-vous présenter l'alternative «cas»? De plus, je dirais que c'est très différent de void *, qui est requis pour toute abstraction sérieuse de structure de données en C.Je ne pense pas qu'il y ait quelqu'un qui s'oppose sérieusement à void *, et vous ne trouverez pas une seule grande base de code sans il.
Eli Bendersky
Re: void *, c'est exactement ce que je veux dire, il n'y a rien de moralement mal non plus. Exemple de commutateur / boîtier ci-dessous. int toto (barre int) {int valeur_retour = 0; int valeur_échec = 0; if (! faire_quelque chose (bar)) {valeur_échec = 1; } else if (! init_stuff (bar)) {valeur_échec = 2; } else if (prepare_stuff (bar)) {{return_value = do_the_thing (bar); cleanup_3 (); } commutateur (valeur_échec) {cas 2: cleanup_2 (); Pause ; cas 1: cleanup_1 (); Pause ; par défaut: break; }}
webmarc
5
@webmarc, désolé mais c'est horrible! Vous venez de simuler entièrement goto avec des étiquettes - en inventant vos propres valeurs non descriptives pour les étiquettes et en implémentant goto avec switch / case. Failure_value = 1 est plus propre que "goto cleanup_something"?
Eli Bendersky
4
J'ai l'impression que vous m'avez installé ici ... votre question est pour un avis, et ce que je préférerais. Pourtant, quand j'ai offert ma réponse, c'est horrible. :-( Quant à votre plainte concernant les noms d'étiquettes, ils sont tout aussi descriptifs que le reste de votre exemple: cleanup_1, foo, bar. Pourquoi attaquez-vous les noms d'étiquettes alors que ce n'est même pas en rapport avec la question?
webmarc
1
Je n'avais aucune intention de «m'installer» et de provoquer des sentiments négatifs, désolé pour ça! Vous avez juste l'impression que votre nouvelle approche vise uniquement la «suppression de goto», sans ajouter plus de clarté. C'est comme si vous aviez réimplémenté ce que goto fait, en utilisant simplement plus de code qui est moins clair. Cette IMHO n'est pas une bonne chose à faire - se débarrasser du goto juste pour le plaisir.
Eli Bendersky
2

GOTO est utile. C'est quelque chose que votre processeur peut faire et c'est pourquoi vous devriez y avoir accès.

Parfois, vous voulez ajouter un petit quelque chose à votre fonction et vous pouvez le faire facilement. Cela peut gagner du temps.

toto
la source
3
Vous n'avez pas besoin d'accéder à tout ce que votre processeur peut faire. La plupart du temps, goto est plus déroutant que l'alternative.
David Thornley
@DavidThornley: Oui, vous avez accès à tous les besoins de votre processeur chose peut faire, sinon, vous perdez votre processeur. Goto est la meilleure instruction en programmation.
Ron Maimon
1

En général, je considérerais le fait qu'un morceau de code pourrait être le plus clairement écrit en utilisant gotocomme un symptôme que le déroulement du programme est probablement plus compliqué que ce qui est généralement souhaitable. Combiner d'autres structures de programme de manière étrange pour éviter l'utilisation de gotopermettrait de traiter le symptôme plutôt que la maladie. Votre exemple particulier pourrait ne pas être trop difficile à mettre en œuvre sans goto:

  faire {
    .. configurer le truc1 qui n'aura besoin d'être nettoyé qu'en cas de sortie anticipée
    si (erreur) rupture;
    faire
    {
      .. configurer le truc2 qui devra être nettoyé en cas de sortie anticipée
      si (erreur) rupture;
      // ***** VOIR LE TEXTE CONCERNANT CETTE LIGNE
    } tandis que (0);
    .. nettoyage chose2;
  } tandis que (0);
  .. nettoyage chose1;

mais si le nettoyage n'était censé se produire que lorsque la fonction a échoué, le gotocas pourrait être traité en plaçant un returnjuste avant la première étiquette cible. Le code ci-dessus nécessiterait l'ajout d'un returnà la ligne marquée par *****.

Dans le scénario "nettoyage même dans le cas normal", je considérerais l'utilisation de gotocomme étant plus claire que les constructions do/ while(0), entre autres parce que les étiquettes cibles elles-mêmes crient pratiquement "REGARDE-MOI" bien plus que les constructions breaket do/ while(0). Pour le cas "nettoyage uniquement en cas d'erreur", l' returninstruction finit par devoir être à peu près au pire endroit possible du point de vue de la lisibilité (les instructions de retour doivent généralement être soit au début d'une fonction, soit à quoi "ressemble" la fin); avoir une returnétiquette juste avant une cible répond à cette qualification beaucoup plus facilement que d'en avoir une juste avant la fin d'une «boucle».

BTW, un scénario dans lequel j'utilise parfois gotopour la gestion des erreurs se trouve dans une switchinstruction, lorsque le code pour plusieurs cas partage le même code d'erreur. Même si mon compilateur serait souvent assez intelligent pour reconnaître que plusieurs cas se terminent par le même code, je pense qu'il est plus clair de dire:

 REPARSE_PACKET:
  commutateur (paquet [0])
  {
    case PKT_THIS_OPERATION:
      if (condition de problème)
        goto PACKET_ERROR;
      ... gérer THIS_OPERATION
      Pause;
    case PKT_THAT_OPERATION:
      if (condition de problème)
        goto PACKET_ERROR;
      ... gérer THAT_OPERATION
      Pause;
    ...
    case PKT_PROCESS_CONDITIONALLY
      si (paquet_longueur <9)
        goto PACKET_ERROR;
      if (condition_packet impliquant le paquet [4])
      {
        packet_length - = 5;
        memmove (paquet, paquet + 5, paquet_longueur);
        goto REPARSE_PACKET;
      }
      autre
      {
        paquet [0] = PKT_CONDITION_SKIPPED;
        paquet [4] = paquet_longueur;
        packet_length = 5;
        packet_status = READY_TO_SEND;
      }
      Pause;
    ...
    défaut:
    {
     PACKET_ERROR:
      packet_error_count ++;
      packet_length = 4;
      paquet [0] = PKT_ERROR;
      packet_status = READY_TO_SEND;
      Pause;
    }
  }   

Bien que l'on puisse remplacer les gotoinstructions par {handle_error(); break;}, et bien que l'on puisse utiliser une boucle do/ while(0)avec continuepour traiter le paquet d'exécution conditionnelle encapsulé, je ne pense pas vraiment que ce soit plus clair que d'utiliser un goto. De plus, bien qu'il soit possible de copier le code de PACKET_ERRORpartout où le goto PACKET_ERRORest utilisé, et alors qu'un compilateur peut écrire le code dupliqué une fois et remplacer la plupart des occurrences par un saut vers cette copie partagée, l'utilisation de gotofacilite la détection des endroits qui place le paquet un peu différemment (par exemple si l'instruction "exécuter conditionnellement" décide de ne pas s'exécuter).

supercat
la source
1

Personnellement, je suis un adepte du "Le pouvoir des dix - 10 règles pour rédiger un code critique de sécurité" .

Je vais inclure un petit extrait de ce texte qui illustre ce que je crois être une bonne idée à propos de goto.


Règle: Restreignez tout le code à des constructions de flux de contrôle très simples - n'utilisez pas d'instructions goto, de constructions setjmp ou longjmp et de récursivité directe ou indirecte.

Justification: un flux de contrôle plus simple se traduit par des capacités de vérification plus fortes et aboutit souvent à une meilleure clarté du code. Le bannissement de la récursivité est peut-être la plus grande surprise ici. Sans récursion, cependant, nous avons la garantie d'avoir un graphe d'appel de fonction acyclique, qui peut être exploité par les analyseurs de code, et peut directement aider à prouver que toutes les exécutions qui devraient être bornées sont en fait bornées. (Notez que cette règle n'exige pas que toutes les fonctions aient un point de retour unique - bien que cela simplifie souvent également le flux de contrôle. Il existe cependant suffisamment de cas où un retour d'erreur précoce est la solution la plus simple.)


Bannir l'utilisation de goto semble mauvais mais:

Si les règles semblent draconiennes au premier abord, gardez à l'esprit qu'elles sont destinées à permettre de vérifier le code là où très littéralement votre vie peut dépendre de son exactitude: code qui sert à contrôler l'avion sur lequel vous volez, la centrale nucléaire. à quelques kilomètres de chez vous, ou du vaisseau spatial qui transporte les astronautes en orbite. Les règles agissent comme la ceinture de sécurité de votre voiture: au début, elles sont peut-être un peu inconfortables, mais après un certain temps, leur utilisation devient une seconde nature et ne pas les utiliser devient inimaginable.

Trevor Boyd Smith
la source
22
Le problème avec ceci est que la manière habituelle de bannir totalement le gotoest d'utiliser un ensemble de booléens «intelligents» dans des if ou des boucles profondément imbriquées. Cela n'aide vraiment pas. Peut-être que vos outils le feront mieux, mais vous ne le ferez pas et vous êtes plus important.
Donal Fellows
1

Je conviens que le nettoyage goto dans l'ordre inverse donné dans la question est le moyen le plus propre de nettoyer les choses dans la plupart des fonctions. Mais je voulais aussi souligner que parfois, vous voulez quand même que votre fonction nettoie. Dans ces cas, j'utilise la variante suivante if if (0) {label:} idiom pour aller au bon moment du processus de nettoyage:

int decode ( char * path_in , char * path_out )
{
  FILE * in , * out ;
  code c ;
  int len ;
  int res = 0  ;
  if ( path_in == NULL )
    in = stdin ;
  else
    {
      if ( ( in = fopen ( path_in , "r" ) ) == NULL )
        goto error_open_file_in ;
    }
  if ( path_out == NULL )
    out = stdout ;
  else
    {
      if ( ( out = fopen ( path_out , "w" ) ) == NULL )
        goto error_open_file_out ;
    }

  if( read_code ( in , & c , & longueur ) )
    goto error_code_construction ;

  if ( decode_h ( in , c , out , longueur ) )
  goto error_decode ;

  if ( 0 ) { error_decode: res = 1 ;}
  free_code ( c ) ;
  if ( 0 ) { error_code_construction: res = 1 ; }
  if ( out != stdout ) fclose ( stdout ) ;
  if ( 0 ) { error_open_file_out: res = 1 ; }
  if ( in != stdin ) fclose ( in ) ;
  if ( 0 ) { error_open_file_in: res = 1 ; }
  return res ;
 }
user1251840
la source
0

Il me semble que cleanup_3devrait faire son nettoyage, puis appeler cleanup_2. De même, cleanup_2devrait faire son nettoyage, puis appeler cleanup_1. Il semble que chaque fois que vous le faites cleanup_[n], cela cleanup_[n-1]est nécessaire, donc cela devrait être la responsabilité de la méthode (de sorte que, par exemple, cleanup_3ne puisse jamais être appelé sans appeler cleanup_2et éventuellement provoquer une fuite.)

Compte tenu de cette approche, au lieu de gotos, vous appelleriez simplement la routine de nettoyage, puis reviendriez.

L' gotoapproche n'est ni fausse ni mauvaise , cependant, il convient de noter que ce n'est pas nécessairement l'approche la plus «propre» (IMHO).

Si vous recherchez les performances optimales, je suppose que la gotosolution est la meilleure. Je m'attends à ce qu'il ne soit pertinent, cependant, que dans quelques applications, critiques pour les performances (par exemple, les pilotes de périphériques, les périphériques intégrés, etc.). Sinon, c'est une micro-optimisation qui a une priorité inférieure à la clarté du code.

Ryan Emerle
la source
4
Cela ne coupera pas - les nettoyages sont spécifiques aux ressources, qui sont allouées dans cet ordre uniquement dans cette routine. Dans d'autres endroits, ils ne sont pas liés, donc appeler l'un de l'autre n'a pas de sens.
Eli Bendersky
0

Je pense que la question ici est fallacieuse par rapport au code donné.

Considérer:

  1. do_something (), init_stuff () et prepare_stuff () semblent savoir s'ils ont échoué, puisqu'ils renvoient soit faux, soit nil dans ce cas.
  2. La responsabilité de la configuration de l'état semble être la responsabilité de ces fonctions, car il n'y a pas d'état configuré directement dans foo ().

Par conséquent: do_something (), init_stuff () et prepare_stuff () devraient faire leur propre nettoyage . Avoir une fonction cleanup_1 () séparée qui nettoie après do_something () rompt la philosophie de l'encapsulation. C'est une mauvaise conception.

S'ils ont fait leur propre nettoyage, alors foo () devient assez simple.

D'autre part. Si foo () créait réellement son propre état qui devait être détruit, alors goto serait approprié.

Simon Woodside
la source
0

Voici ce que j'ai préféré:

bool do_something(void **ptr1, void **ptr2)
{
    if (!ptr1 || !ptr2) {
        err("Missing arguments");
        return false;
    }
    bool ret = false;

    //Pointers must be initialized as NULL
    void *some_pointer = NULL, *another_pointer = NULL;

    if (allocate_some_stuff(&some_pointer) != STUFF_OK) {
        err("allocate_some_stuff step1 failed, abort");
        goto out;
    }
    if (allocate_some_stuff(&another_pointer) != STUFF_OK) {
        err("allocate_some_stuff step 2 failed, abort");
        goto out;
    }

    void *some_temporary_malloc = malloc(1000);

    //Do something with the data here
    info("do_something OK");

    ret = true;

    // Assign outputs only on success so we don't end up with
    // dangling pointers
    *ptr1 = some_pointer;
    *ptr2 = another_pointer;
out:
    if (!ret) {
        //We are returning an error, clean up everything
        //deallocate_some_stuff is a NO-OP if pointer is NULL
        deallocate_some_stuff(some_pointer);
        deallocate_some_stuff(another_pointer);
    }
    //this needs to be freed every time
    free(some_temporary_malloc);
    return ret;
}
Mikko Korkalo
la source
0

Ancienne discussion, cependant ... qu'en est-il de l'utilisation de "flèche anti-motif" et d'encapsuler plus tard chaque niveau imbriqué dans une fonction statique en ligne? Le code a l'air propre, il est optimal (lorsque les optimisations sont activées) et aucun goto n'est utilisé. En bref, divisez pour conquérir. Ci-dessous un exemple:

static inline int foo_2(int bar)
{
    int return_value = 0;
    if ( prepare_stuff( bar ) ) {
        return_value = do_the_thing( bar );
    }
    cleanup_3();
    return return_value;
}

static inline int foo_1(int bar)
{
    int return_value = 0;
    if ( init_stuff( bar ) ) {
        return_value = foo_2(bar);
    }
    cleanup_2();
    return return_value;
}

int foo(int bar)
{
    int return_value = 0;
    if (do_something(bar)) {
        return_value = foo_1(bar);
    }
    cleanup_1();
    return return_value;
}

En termes d'espace, nous créons trois fois la variable dans la pile, ce qui n'est pas bon, mais cela disparaît lors de la compilation avec -O2 en supprimant la variable de la pile et en utilisant un registre dans cet exemple simple. Ce que j'ai obtenu du bloc ci-dessus gcc -S -O2 test.cétait le suivant:

    .section    __TEXT,__text,regular,pure_instructions
    .macosx_version_min 10, 13
    .globl  _foo                    ## -- Begin function foo
    .p2align    4, 0x90
_foo:                                   ## @foo
    .cfi_startproc
## %bb.0:
    pushq   %rbp
    .cfi_def_cfa_offset 16
    .cfi_offset %rbp, -16
    movq    %rsp, %rbp
    .cfi_def_cfa_register %rbp
    pushq   %r14
    pushq   %rbx
    .cfi_offset %rbx, -32
    .cfi_offset %r14, -24
    movl    %edi, %ebx
    xorl    %r14d, %r14d
    xorl    %eax, %eax
    callq   _do_something
    testl   %eax, %eax
    je  LBB0_6
## %bb.1:
    xorl    %r14d, %r14d
    xorl    %eax, %eax
    movl    %ebx, %edi
    callq   _init_stuff
    testl   %eax, %eax
    je  LBB0_5
## %bb.2:
    xorl    %r14d, %r14d
    xorl    %eax, %eax
    movl    %ebx, %edi
    callq   _prepare_stuff
    testl   %eax, %eax
    je  LBB0_4
## %bb.3:
    xorl    %eax, %eax
    movl    %ebx, %edi
    callq   _do_the_thing
    movl    %eax, %r14d
LBB0_4:
    xorl    %eax, %eax
    callq   _cleanup_3
LBB0_5:
    xorl    %eax, %eax
    callq   _cleanup_2
LBB0_6:
    xorl    %eax, %eax
    callq   _cleanup_1
    movl    %r14d, %eax
    popq    %rbx
    popq    %r14
    popq    %rbp
    retq
    .cfi_endproc
                                        ## -- End function

.subsections_via_symbols
Alejandro Visiedo García
la source
-1

Je préfère utiliser la technique décrite dans l'exemple suivant ...

struct lnode *insert(char *data, int len, struct lnode *list) {
    struct lnode *p, *q;
    uint8_t good;
    struct {
            uint8_t alloc_node : 1;
            uint8_t alloc_str : 1;
    } cleanup = { 0, 0 };

    // allocate node.
    p = (struct lnode *)malloc(sizeof(struct lnode));
    good = cleanup.alloc_node = (p != NULL);

    // good? then allocate str
    if (good) {
            p->str = (char *)malloc(sizeof(char)*len);
            good = cleanup.alloc_str = (p->str != NULL);
    }

    // good? copy data
    if(good) {
            memcpy ( p->str, data, len );
    }

    // still good? insert in list
    if(good) {
            if(NULL == list) {
                    p->next = NULL;
                    list = p;
            } else {
                    q = list;
                    while(q->next != NULL && good) {
                            // duplicate found--not good
                            good = (strcmp(q->str,p->str) != 0);
                            q = q->next;
                    }
                    if (good) {
                            p->next = q->next;
                            q->next = p;
                    }
            }
    }

    // not-good? cleanup.
    if(!good) {
            if(cleanup.alloc_str)   free(p->str);
            if(cleanup.alloc_node)  free(p);
    }

    // good? return list or else return NULL
    return (good? list: NULL);

}

source: http://blog.staila.com/?p=114

Nitin Kunal
la source
2
le code drapeau et l'anti-motif de flèche (tous deux présentés dans votre exemple) sont tous deux des éléments qui compliquent inutilement le code. Il n'y a aucune justification en dehors de "goto is evil" pour les utiliser.
KingRadical
-1

Nous utilisons la Daynix CStepsbibliothèque comme une autre solution pour le " problème goto " dans les fonctions init.
Voir ici et ici .

Daynix Computing LTD
la source
3
l'abus de macro (comme avec CSteps) est bien pire qu'une utilisation judicieuse degoto
KingRadical