A Question of Style

Discussion in 'C Programming' started by Geoff, Sep 21, 2013.

  1. Geoff

    Geoff Guest

    I found this code in a Quake 2 project. Would you call this a case of
    premature optimization or a case of single-returnitis in the face of a
    necessarily multi-return function. I am referring to the multiple
    gotos in the function designed to bypass a single line of code.

    void TeamplaySpawnEntities (char *mapname, char *entities, char
    *spawnpoint)
    {
    FILE *f;
    char szFile[MAX_QPATH];
    int nEntSize, nRead;
    char *pszCustomEnt;

    // If teamplay is off, do the normal thing.
    if (!ctf->value)
    {
    SpawnEntities (mapname, entities, spawnpoint);
    return;
    }

    // Create the pathname to the entity file.
    Com_sprintf (szFile, sizeof (szFile), "%s/ent/%s.ent",
    gamedir->string, mapname);

    // Try to open it.
    f = fopen (szFile, "rb");
    if (!f)
    {
    // No custom entity file, so just use the default.
    SpawnEntities (mapname, entities, spawnpoint);
    return;
    }

    // Get the size of the file.
    if (fseek (f, 0, SEEK_END) != 0)
    {
    gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    goto errClose;
    }
    nEntSize = ftell (f);
    if (nEntSize < 0)
    {
    gi.dprintf ("TeamplaySpawnEntities(): ftell %s (%d)\n",
    szFile, nEntSize);
    goto errClose;
    }
    if (fseek (f, 0, SEEK_SET) != 0)
    {
    gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    goto errClose;
    }

    // Create a buffer and read the custom entity file.
    pszCustomEnt = malloc (nEntSize + 1);
    if (!pszCustomEnt)
    {
    gi.dprintf ("TeamplaySpawnEntities(): malloc\n");
    goto errClose;
    }
    nRead = fread (pszCustomEnt, 1, nEntSize, f);
    if (nRead != nEntSize)
    {
    gi.dprintf ("TeamplaySpawnEntities(): fread %s (%d/%d)\n",
    szFile, nRead, nEntSize);
    goto errFree;
    }

    // Null-terminate the string.
    pszCustomEnt[nEntSize] = '\0';

    // Now spawn *these* entities!
    SpawnEntities (mapname, pszCustomEnt, spawnpoint);

    // Clean up.
    errFree:
    free (pszCustomEnt);
    errClose:
    fclose (f);
    }
     
    Geoff, Sep 21, 2013
    #1
    1. Advertisements

  2. Geoff

    James Kuyper Guest

    malloc() and fopen() are two examples of cases where I believe in the
    single-return dogma, but I wouldn't use gotos for that purpose. I'll
    re-write the code to show how I would have dealt with this particular
    issue. I won't change any of the other aspects of the code that are
    different from the way I would have written them (of which there are many).

    void TeamplaySpawnEntities (char *mapname, char *entities, char
    *spawnpoint)
    {
    FILE *f;
    char szFile[MAX_QPATH];


    // If teamplay is off, do the normal thing.
    if (!ctf->value)
    {
    SpawnEntities (mapname, entities, spawnpoint);
    return;
    }

    // Create the pathname to the entity file.
    Com_sprintf (szFile, sizeof (szFile), "%s/ent/%s.ent",
    gamedir->string, mapname);

    // Try to open it.
    f = fopen (szFile, "rb");
    if (!f)
    {
    // No custom entity file, so just use the default.
    SpawnEntities (mapname, entities, spawnpoint);
    return;
    }

    // Get the size of the file.
    if (fseek (f, 0, SEEK_END) != 0)
    gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    else
    {
    int nEntSize = ftell (f);

    if (nEntSize < 0)
    gi.dprintf ("TeamplaySpawnEntities(): ftell %s (%d)\n",
    szFile, nEntSize);
    else if (fseek (f, 0, SEEK_SET) != 0)
    gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    else
    {
    // Create a buffer and read the custom entity file.
    char *pszCustomEnt = malloc (nEntSize + 1);

    if (!pszCustomEnt)
    gi.dprintf ("TeamplaySpawnEntities(): malloc\n");

    else
    {
    int nRead = fread (pszCustomEnt, 1, nEntSize, f);

    if (nRead != nEntSize)
    gi.dprintf (
    "TeamplaySpawnEntities(): fread %s (%d/%d)\n",
    szFile, nRead, nEntSize);
    else
    {
    // Null-terminate the string.
    pszCustomEnt[nEntSize] = '\0';

    // Now spawn *these* entities!
    SpawnEntities (mapname, pszCustomEnt, spawnpoint);
    }

    // Clean up.
    free (pszCustomEnt);
    }
    }
    }
    fclose (f);
    }

    The key point is that it's easy to determine, by examining this code,
    that fclose() will be called if, and only if, fopen() succeeds, and that
    free() will be called if, and only if, malloc() succeeds (assuming that
    all subroutines actually return). That would not have been true if the
    goto's had simply been replaced by return statements.
     
    James Kuyper, Sep 22, 2013
    #2
    1. Advertisements

  3. Geoff

    lab17 Guest

    在 2013å¹´9月22日星期日UTC+8上åˆ6æ—¶43分35秒,Geoff写é“:
    Maybe you can choice " do {/*Do some thing*/} while(0); " style to void goto statement, then you can use break instead of goto.

    ///////////////////////////////////////////////////////////
    do
    {
    // Get the size of the file.
    if (fseek (f, 0, SEEK_END) != 0)
    {
    gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    break;
    }
    nEntSize = ftell (f);
    if (nEntSize < 0)
    {
    gi.dprintf ("TeamplaySpawnEntities(): ftell %s (%d)\n",
    szFile, nEntSize);
    break;
    }
    if (fseek (f, 0, SEEK_SET) != 0)
    {
    gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    break;
    }

    // Create a buffer and read the custom entity file.
    pszCustomEnt = malloc (nEntSize + 1);
    if (!pszCustomEnt)
    {
    gi.dprintf ("TeamplaySpawnEntities(): malloc\n");
    break;
    }
    }while (0);

    fclose(f);

    /////////////////////////////////////////////////////

    Thanks,
    Jianpo
     
    lab17, Sep 22, 2013
    #3
  4. Geoff

    Rosario1903 Guest

    the problem is not "the use of goto"
    it is how indent the program
    how to use art for show better what a program does
     
    Rosario1903, Sep 22, 2013
    #4
  5. Geoff

    Rosario1903 Guest

    in your code afther ps... = malloc(...) etc
    you forget there is this code too

    etc
     
    Rosario1903, Sep 22, 2013
    #5
  6. But you've taken a small part of the original. It's much harder to fake
    the effect of the original gotos using break because there are two
    "error" targets (though they are not needed).
    Why? The portion you've chosen to re-write is just a simple
    if-then-else.

    In cases like this it's handy that free(NULL) is permitted. It's a
    shame that fclose(NULL) is not also permitted, but a simple 'if' at the
    point of closing can often solve these messy problems.

    For example, the structure of the original code was:

    if (test args) {
    take default action;
    return;
    }

    if ((f = fopen(...) == NULL) {
    take default action;
    return;
    }

    if (fseek(...) != 0) {
    print error;
    }
    else if ((p = ftell(f)) < 0) {
    print error;
    }
    else if (fseek(...) != 0) {
    print error;
    }
    else if ((buf = malloc(...)) == NULL) {
    print error;
    }
    else {
    do the real work;
    }

    free(buf);
    if (f) fclose(f);

    I think that's clearer and not at all contrived.
     
    Ben Bacarisse, Sep 22, 2013
    #6
  7. Geoff

    Rosario1903 Guest

    i'm sorry, but it is not matter of styles...
    it is about what it is easier...

    in a function there would be 2 place of exit

    the first one for error [i would put it with a label for check if arg
    are ok in the biginning of the routine] and the second in the end for
    say all ok

    but i think could exist good functions that are ok and have many
    return points too

    all depend, some function are good with 2 place of exit some function
    are good with 1000 point of exit

    for the return to theoretic argument:
    the path for error can have differt goto-labels that can reverse free
    all resource in case of error, with code very easy to read...

    in difference of your code...

    i would write something as:


    void TeamplaySpawnEntities(char* mapname, char* entities,
    char* spawnpoint)
    {FILE *f;
    char szFile[MAX_QPATH];
    int nEntSize, nRead;
    char *pszCustomEnt;

    // If teamplay is off, do the normal thing.
    if(ctf->value==0)
    {rexit1:
    SpawnEntities (mapname, entities, spawnpoint);
    rexit : return;
    }

    // Create the pathname to the entity file.
    Com_sprintf(szFile, sizeof (szFile), "%s/ent/%s.ent",
    gamedir->string, mapname);
    // Try to open it.
    f=fopen (szFile, "rb");

    // No custom entity file, so just use the default.
    if(f==0) goto rexit1;

    // Get the size of the file.
    if (fseek (f, 0, SEEK_END) != 0)
    {gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    errClose:
    fclose (f);
    goto rexit;
    }

    nEntSize = ftell (f);
    if (nEntSize < 0)
    {gi.dprintf("TeamplaySpawnEntities(): ftell %s (%d)\n",
    szFile, nEntSize);
    goto errClose;
    }

    if (fseek(f, 0, SEEK_SET) != 0)
    {gi.dprintf ("TeamplaySpawnEntities(): fseek %s\n", szFile);
    goto errClose;
    }

    // Create a buffer and read the custom entity file.
    pszCustomEnt = malloc (nEntSize + 1);
    if (pszCustomEnt==0)
    {gi.dprintf ("TeamplaySpawnEntities(): malloc\n");
    goto errClose;
    }

    nRead = fread(pszCustomEnt, 1, nEntSize, f);
    if(nRead != nEntSize)
    {gi.dprintf("TeamplaySpawnEntities(): fread %s (%d/%d)\n",
    szFile, nRead, nEntSize);
    errFree:
    free (pszCustomEnt); goto errClose;
    }

    // Null-terminate the string.
    pszCustomEnt[nEntSize] = '\0';

    // Now spawn *these* entities!
    SpawnEntities (mapname, pszCustomEnt, spawnpoint);

    free (pszCustomEnt);
    fclose (f);
    }

    but i think it would be better return one int
    with something as R fclose(f);...
     
    Rosario1903, Sep 22, 2013
    #7
  8. Geoff

    Rosario1903 Guest

    for example this:

    R== return

    i32 cmpA(const void* a, const void* b)
    {u32 *aa, *bb;
    u8 *ac, *bc;
    i32 i;
    aa=(u32*)a; bb=(u32*)b;
    ac=(u8*)(aa+1); bc=(u8*)(bb+1);
    if(ac[0]>bc[0]) R -1;
    if(ac[0]<bc[0]) R 1;
    if(ac[1]>bc[1]) R -1;
    if(ac[1]<bc[1]) R 1;
    if(ac[2]>bc[2]) R -1;
    if(ac[2]<bc[2]) R 1;
    if(ac[3]>bc[3]) R -1;
    if(ac[3]<bc[3]) R 1;
    if(ac[4]>bc[4]) R -1;
    if(ac[4]<bc[4]) R 1;
    if(ac[5]>bc[5]) R -1;
    if(ac[5]<bc[5]) R 1;
    if(ac[6]>bc[6]) R -1;
    if(ac[6]<bc[6]) R 1;

    R 0;
    }
     
    Rosario1903, Sep 22, 2013
    #8
  9. Geoff

    Tim Rentsch Guest

    I would call it atrocious code in need of re-writing. A
    classic example of trying to do too much in a single function,
    with hard-to-follow control flow being just one symptom. It
    seems much better to divide the original function up so there
    are a couple of subfunctions, with the smaller pieces being
    more easily comprehensible. One way of doing that follows
    (take out the 'static' keyord in the function parameter
    declarations if using C89). Note the top-level function
    definition shows immediately that the file is closed and the
    associated allocated memory is freed if the file open was
    successful.


    static FILE *open_entity_file( char name[ static MAX_QPATH ], char *mapname );
    static char *file_contents( FILE *f, char *name, const char *who );


    void
    TeamplaySpawnEntities( char *mapname, char *entities, char *spawnpoint ){
    char file_name[ MAX_QPATH ];
    FILE *f = open_entity_file( file_name, mapname );
    char *e = f ? file_contents( f, file_name, __func__ ) : entities;

    if( e || !f ) SpawnEntities( mapname, e, spawnpoint );

    if( f ) free( e ), fclose( f );
    }


    FILE *
    open_entity_file( char name[ static MAX_QPATH ], char *mapname ){
    if( ! ctf->value ) return 0;

    Com_sprintf( name, MAX_QPATH, "%s/ent/%s.ent", gamedir->string, mapname );
    return fopen( name, "rb" );
    }


    char *
    file_contents( FILE *f, char *name, const char *who ){
    long size, n;
    char *result = 0;

    if( fseek( f, 0, SEEK_END ) != 0 ){
    gi.dprintf( "%s(): fseek %s\n", who, name );

    } else if( size = ftell( f ), size < 0 ){
    gi.dprintf( "%s(): ftell %s (%d)\n", who, name, size );

    } else if( fseek( f, 0, SEEK_SET ) != 0 ){
    gi.dprintf( "%s(): fseek %s\n", who, name );

    } else if( result = malloc( size+1 ), ! result ){
    gi.dprintf( "%s(): malloc\n", who );

    } else if( n = fread( result, 1, size, f ), n != size ){
    gi.dprintf( "%s(): fread %s (%d/%d)\n", who, name, n, size );
    free( result ), result = 0;
    }

    return result ? result[size] = 0, result : 0;
    }
     
    Tim Rentsch, Sep 23, 2013
    #9
  10. You might want to look into this fantastic new conecept in one of the
    most recent C-standards, I think it was called "loops" or something
    along the lines. Don't remember exactly, however, such an obscure thing.

    Side note to myself: Commit suicide should I ever end up programming in
    one team with Rosario.

    Cheers,
    Joe

    --
    Ah, der neueste und bis heute genialste Streich unsere großen
    Kosmologen: Die Geheim-Vorhersage.
    - Karl Kaos über Rüdiger Thomas in dsa <hidbv3$om2$>
     
    Johannes Bauer, Sep 23, 2013
    #10
  11. Geoff

    Geoff Guest

    Joe, I think you forget, the last Italian who tried to loop grounded
    his ship on a rock.
     
    Geoff, Sep 23, 2013
    #11
  12. Really? In the 21st century? In a technical news group? A quip
    predicated on some supposed failing of Italians?
     
    Ben Bacarisse, Sep 23, 2013
    #12
  13. Geoff

    Rosario1903 Guest

    if i understant well i would be like skettino from napule
     
    Rosario1903, Sep 23, 2013
    #13
  14. Geoff

    BartC Guest

    If he's talking about Schettino (the cruise ship captain), then that was an
    actual failing of one Italian (ignoring the minor quibble that the trial is
    still on-going).
     
    BartC, Sep 23, 2013
    #14
  15. Geoff

    James Kuyper Guest

    True - but Geoff's comment implied that the failings of both Shettino
    and Rosario were connected to the fact that they are Italian, and that's
    what's troubling about it. I've seen many non-Italians post messages to
    this newsgroups that made it clear that the were as least as foolish as
    Rosario. I cannot, off-hand, come up with any examples where a captain
    caused so much damage for such a foolish reason as Schettino's, but I'm
    still reasonably sure that such lethal foolishness is not
    disproportionately perpetrated by Italians.
     
    James Kuyper, Sep 24, 2013
    #15
  16. Geoff

    Geoff Guest

    My original post had the goal of soliciting opinions about the
    utilization of gotos vs simply writing fclose(f), return; in place of
    goto errClose; and free(x), fclose(f), return; in place of errFree.

    As written, there are three exits, return and the two above. If you
    were going to write it straight through, those would be my choices and
    I would probably want to start there on a rewrite. It seems to me a
    compiler seeing code taking the same exits out of function ought to
    merge them into the same jmp target in the object code but that's
    making some big assumptions about the intelligence of the compiler.

    I'd be interested in seeing comments about the maintainability of
    several exits vs stateful single exit.

    I agree the function is too large as-is and splitting it up would be
    preferable. I thought Ben's if (f) fclose(f); was an excellent
    solution to eliminating the errClose's.
     
    Geoff, Sep 24, 2013
    #16
  17. Geoff

    James Kuyper Guest

    On 09/24/2013 09:32 AM, Geoff wrote:
    ....
    I strongly believe, based upon personal experience, that having a single
    fclose() for each fopen() and a single free() for each malloc() does
    improve maintainability. It's not always possible, particularly when
    dynamically allocating memory for a complicated tree structure.
    That's workable, and I've written code that relies upon the fact that
    free(NULL) is legal, which is basically the same idea. However, I don't
    like it, because it involves two separate tests for if(f), when a simple
    restructuring of the logic allows a single if(f) to be sufficient.
     
    James Kuyper, Sep 24, 2013
    #17
  18. Geoff

    Tim Rentsch Guest

    If you find yourself having to consider such questions it's
    almost always a sign that the functions being defined are
    too large and should be decomposed.

    The compilers being able to collapse common code sequences
    is (most often) an irrelevant consideration.

    The big downside of replicating code at earlier return points is
    all the usual problems that come from replicating code, eg, the
    multiple copies might get out of sync. Sometimes it takes a
    substantial effort to follow the Factoring Principle, but IME
    that effort is almost always worth the trouble.

    Personally I would probably put both use of 'goto' and having
    replicated code sequences in the category of "guilty until proven
    innocent" - it's not necessarily wrong to resort to these but one
    better be prepared to defend any use in the circumstances in
    question. Having said that, there are fairly widely known and
    easy to understand patterns for using goto in a structured way,
    for the particular case of unwinding resource allocation in
    preparation for function return.
     
    Tim Rentsch, Sep 25, 2013
    #18
  19. Geoff

    Seebs Guest

    Pretty sure this was a historical joke, not a joke about a failing of
    Italians as a class. I tend to allow for a certain amount of "references
    to famous people" as not really being ethnic humor of the traditional
    (and, these days, not so popular) kind.

    -s
     
    Seebs, Sep 28, 2013
    #19
  20. My pattern is to have a create and a kill. The create allocates the
    structure, the kill destroys it (so C++ constructors / destructors,
    in effect). Now you write the kill so that it's robust to partly created
    structures. So it always does a no-op for a null. But it can't
    distinguish a valid pointer from an uninitialised garbage pointer.
    So you need to be careful to set all pointer members to null immediately
    after allocating them.
    Normally the bulk of memory allocations are in the create function.
    So you just need a call to the kill and to return NULL on allocation
    failure. However you might need to allocate temporary buffers. So it's
    best to have an error_exit label at the bottom of the function, which
    destroys everything and returns NULL. (Occasionally other errors are
    possible e.g. file IO or, for complex creates that take grammars, parse
    errors).

    A memory allocation error in a non-create function you have to handle
    as the situation demands. Sometimes you want to destroy the object,
    sometimes you want to return an error code. However you can't
    easily destroy the object because caller has "got" it. So you need
    to mark it as invalid in some way. That's not always possible, or it
    might be messy (freeing an internal array and setting its length to
    zero for example).
     
    Malcolm McLean, Sep 28, 2013
    #20
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.