A Question of Style

[Geoff]

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);
}

 

[James Kuyper]

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);
}

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.

 

[lab17]

在 2013å¹´9月22日星期日UTC+8上åˆ6æ—¶43分35秒，Geoff写é“：

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);

}

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

 

[Rosario1903]

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.

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]

? 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

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

etc

 

[Ben Bacarisse]

在 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.

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).

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);

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.

 

[Rosario1903]

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.

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]

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

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;
}

 

[Tim Rentsch]

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. [snip 74 line function definition]

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;
}

 

[Johannes Bauer]

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;
}

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

--

Zumindest nicht öffentlich!

Ah, der neueste und bis heute genialste Streich unsere großen
Kosmologen: Die Geheim-Vorhersage.
- Karl Kaos über Rüdiger Thomas in dsa <[email protected]>

 

[Geoff]

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;
}

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

Joe, I think you forget, the last Italian who tried to loop grounded
his ship on a rock.

 

[Ben Bacarisse]

Joe, I think you forget, the last Italian who tried to loop grounded
his ship on a rock.

Really? In the 21st century? In a technical news group? A quip
predicated on some supposed failing of Italians?

 

[Rosario1903]

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;
}

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

Joe, I think you forget, the last Italian who tried to loop grounded
his ship on a rock.

if i understant well i would be like skettino from napule

 

[BartC]

Really? In the 21st century? In a technical news group? A quip
predicated on some supposed failing of Italians?

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).

 

[James Kuyper]

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).

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.

 

[Geoff]

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. [snip 74 line function definition]

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.

That was my thought too, though I would write it in a very different
style from your code.

Code that involves "goto" can almost always be better written using
multiple functions and some sort of state variables to give clearer and
more maintainable code.

You generally see such "goto" style in older code, as a way of getting
faster code. More modern compliers and more modern C standards (giving
explicit and automatic "inline") can roll structured code with multiple
functions into a single block with goto's generated in the generated code.

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.

 

[James Kuyper]

On 09/24/2013 09:32 AM, Geoff wrote:
....

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 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.

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.

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.

 

[Tim Rentsch]

[snip]

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.

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.

 

[Seebs]

Really? In the 21st century? In a technical news group? A quip
predicated on some supposed failing of Italians?

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

 

[Malcolm McLean]

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.

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).