Error returns and repeated code, GOTOs, etc.

[mike3]

Hi.

I've got some stuff that looks like this:

---
ErrorCode MyFunc()
{
BigNum a, b, c, d;
ErrorCode rv;

rv = BigNum_Initialize(&a);
if(rv != ERROR_SUCCESS)
return(rv);

rv = BigNum_Initialize(&b);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&a); /* and what if THIS fails??? */
return(rv);
}

rv = BigNum_Initialize(&c);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&b);
BigNum_Free(&a);
return(rv);
}

rv = BigNum_Initialize(&d);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&c);
BigNum_Free(&b);
BigNum_Free(&a);
return(rv);
}

/* do some math */
rv = BigNum_Set(&b, 3);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&d);
BigNum_Free(&c);
BigNum_Free(&b);
BigNum_Free(&a);
return(rv);
}

...

/* free buffers */
rv = BigNum_Free(&d);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&c);
BigNum_Free(&b);
BigNum_Free(&a);
return(rv);
}

rv = BigNum_Free(&c);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&b);
BigNum_Free(&a);
return(rv);
}

rv = BigNum_Free(&b);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&a);
return(rv);
}

rv = BigNum_Free(&a);
if(rv != ERROR_SUCCESS)
return(rv);

return(ERROR_SUCCESS);
}
---

But this is nasty, with all those duplicated "free" blocks. What can
be done to relieve this code duplication? It's ugly, it hurts my
freaking eyes and my nose is screaming for relief. And it hurts
maintainability to no end -- what if we decide to add more capability
to MyFunc() that requires additional bignums? Oy... We'd have to
update _all those blocks_.

I was thinking about using a "goto", but Gotos are Bad, aren't they? I
can't believe I may have to use a goto! What about Djikstra's famous
letter? Is it possible that perhaps a goto may be GOOD here? Because
with a goto this would all seem much simpler. If we violate Djikstra,
we get:

---

ErrorCode MyFunc()
{
BigNum a, b, c, d;
ErrorCode rv;

rv = BigNum_Initialize(&a); if(rv != ERROR_SUCCESS) goto
fail0;
rv = BigNum_Initialize(&b); if(rv != ERROR_SUCCESS) goto
fail1;
rv = BigNum_Initialize(&c); if(rv != ERROR_SUCCESS) goto
fail2;
rv = BigNum_Initialize(&d); if(rv != ERROR_SUCCESS) goto
fail3;

/* do some math */
rv = BigNum_Set(&b, 3); if(rv != ERROR_SUCCESS) goto fail4;

...

/* free buffers */
rv = BigNum_Free(&d); if(rv != ERROR_SUCCESS) goto fail3;
rv = BigNum_Free(&c); if(rv != ERROR_SUCCESS) goto fail2;
rv = BigNum_Free(&b); if(rv != ERROR_SUCCESS) goto fail1;
rv = BigNum_Free(&a); if(rv != ERROR_SUCCESS) goto fail0;

return(ERROR_SUCCESS);

fail4: BigNum_Free(&d);
fail3: BigNum_Free(&c);
fail2: BigNum_Free(&b);
fail1: BigNum_Free(&a);
fail0: return(rv);
}

 

[Ike Naar]

Hi.

I've got some stuff that looks like this:

[stuff snipped]

But this is nasty, with all those duplicated "free" blocks. What can
be done to relieve this code duplication? It's ugly, it hurts my
freaking eyes and my nose is screaming for relief. And it hurts
maintainability to no end -- what if we decide to add more capability
to MyFunc() that requires additional bignums? Oy... We'd have to
update _all those blocks_.

ErrorCode MyFunc(void)
{
BigNum a;
ErrorCode rv = BigNum_Initialize(&a);
if (rv == ERROR_SUCCESS)
{
BigNum b;
rv = BigNum_Initialize(&b);
if (rv == ERROR_SUCCESS)
{
BigNum c;
rv = BigNum_Initialize(&c);
if (rv == ERROR_SUCCESS)
{
BigNum d;
rv = BigNum_Initialize(&d);
if (rv == ERROR_SUCCESS)
{
/* do some math */
rv = BigNum_Set(&b, 3);
if (rv == ERROR_SUCCESS)
{
/* ... */
}
BigNum_Free(&d);
}
BigNum_Free(&c);
}
BigNum_Free(&b);
}
BigNum_Free(&a);
}
return rv;
}

 

[Nils M Holm]

But this is nasty, with all those duplicated "free" blocks. What can
be done to relieve this code duplication? [...]

Add a garbage collector for BigNums.

 

[Malcolm McLean]

בת×ריך ×™×•× ×¨×‘×™×¢×™, 30 במ××™ 2012 06:44:06 UTC+1, מ×ת mike3:

But we've violated Djikstra and we've used a "bad" goto. How would
_you_ write MyFunc() above?

I use gotos for handling memory allocation failures, but almost never for any other purpose.

 

[mike3]

בת×ריך ×™×•× ×¨×‘×™×¢×™, 30 במ××™ 2012 06:44:06 UTC+1, מ×ת mike3:


I use gotos for handling memory allocation failures, but almost never forany other purpose.

So what would you do in the above circumstance?

 

[MarkBluemel]

But we've violated Djikstra and we've used a "bad" goto. How would
_you_ write MyFunc() above?

I personally don't like the infinitely indented if approach...

In C, I think it's probably OK to use goto for "exception handling".

If you have some way of indicating that a BigNum is uninitialised (e.g.
a BIGNUM_NULL macro) and you make BigNum_Free() set the BigNum to this
value, you could use something like this - goto-rich but less repetitive:-

ErrorCode MyFunc()
{
BigNum a = BIGNUM_NULL;
BigNum b = BIGNUM_NULL;
BigNum c = BIGNUM_NULL;
BigNum d = BIGNUM_NULL;
ErrorCode rv;

rv = BigNum_Initialize(&a);
if(rv != ERROR_SUCCESS)
goto failed;

rv = BigNum_Initialize(&b);
if(rv != ERROR_SUCCESS)
goto failed;

rv = BigNum_Initialize(&c);
if(rv != ERROR_SUCCESS)
goto failed;

rv = BigNum_Initialize(&d);
if(rv != ERROR_SUCCESS)
goto failed;

/* do some math */
rv = BigNum_Set(&b, 3);
if(rv != ERROR_SUCCESS)
goto failed;

...
/* free buffers */
rv = BigNum_Free(&d);
if(rv != ERROR_SUCCESS)
goto failed;

rv = BigNum_Free(&c);
if(rv != ERROR_SUCCESS)
goto failed;

rv = BigNum_Free(&b);
if(rv != ERROR_SUCCESS)
goto failed;

rv = BigNum_Free(&a);
if(rv != ERROR_SUCCESS)
goto failed;

return(ERROR_SUCCESS);

failed: /* we'll try and clean up, but won't care too much if it fails */
if (d != BIGNUM_NULL)
BigNum_Free(&d);
if (c != BIGNUM_NULL)
BigNum_Free(&c);
if (b != BIGNUM_NULL)
BigNum_Free(&b);
if (a != BIGNUM_NULL)
BigNum_Free(&a);

return(rv);
}

 

[Willem]

MarkBluemel wrote:
) On 05/30/2012 06:44 AM, mike3 wrote:
)
)> But we've violated Djikstra and we've used a "bad" goto. How would
)> _you_ write MyFunc() above?
)
) I personally don't like the infinitely indented if approach...
)
) In C, I think it's probably OK to use goto for "exception handling".
)
) If you have some way of indicating that a BigNum is uninitialised (e.g.
) a BIGNUM_NULL macro) and you make BigNum_Free() set the BigNum to this
) value, you could use something like this - goto-rich but less repetitive:-

Rewritten to get rid of the gotos:

ErrorCode DoMath(BigNum a, BigNum b. BigNum c, BigNum d)
{
ErrorCode rv;
/* do some math */
rv = BigNum_Set(&b, 3);
if (rv != ERROR_SUCCESS) return rv;

...

return ERROR_SUCCESS;
}
ErrorCode MyFunc()
{
BigNum a = BIGNUM_NULL;
BigNum b = BIGNUM_NULL;
BigNum c = BIGNUM_NULL;
BigNum d = BIGNUM_NULL;
ErrorCode rv = ERROR_SUCCESS;

if (rv == ERROR_SUCCESS) rv = BigNum_Initialize(&a);
if (rv == ERROR_SUCCESS) rv = BigNum_Initialize(&b);
if (rv == ERROR_SUCCESS) rv = BigNum_Initialize(&c);
if (rv == ERROR_SUCCESS) rv = BigNum_Initialize(&d);

if (rv == ERROR_SUCCESS) rv = DoMath(a, b, c, d);

/* BigNum_Free should check for BIGNUM_NULL itself */
BigNum_Free(&d);
BigNum_Free(&c);
BigNum_Free(&b);
BigNum_Free(&a);

return rv;
}

If you want to be more concise, you can make ERROR_SUCCESS equal 0,
and then you can simply do:

rv = BigNum_Initialize(&a) || ... || BigNum_Set(&b, 3) || BigNum_AddTo(&a, &b) || ... ;

Which will short-circuit at the first failure.

SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[Ian Collins]

Hi.

I've got some stuff that looks like this:

---
ErrorCode MyFunc()
{
BigNum a, b, c, d;
ErrorCode rv;

rv = BigNum_Initialize(&a);
if(rv != ERROR_SUCCESS)
return(rv);

rv = BigNum_Initialize(&b);
if(rv != ERROR_SUCCESS)
{
BigNum_Free(&a); /* and what if THIS fails??? */
return(rv);
}

<snip lots more of the same>

I know I'm inviting flames, but this really is a classic case where C++
if you have access to it can be used as a better C....

 

[Malcolm McLean]

בת×ריך ×™×•× ×¨×‘×™×¢×™, 30 במ××™ 2012 10:50:10 UTC+1, מ×ת Ian Collins:

I know I'm inviting flames, but this really is a classic case where C++
if you have access to it can be used as a better C....

You want

rv = BigNum_Initilaize(&a);
if(rv)
goto error_exit;
rv = BigNum_Add(&a, &b, &c);
if(!rv)
goto error_exit;

it's slightly messier than exceptions, but unlike exceptions it's easier totest and harder to abuse.
You don't want to be defining the symbol,ERROR_SUCCESS in a bignum.h header, and you don't want bignum.c dependent on myerrorcodes.h. However -1 for out of memory is pretty standard.

 

[Ian Collins]

בת×ריך ×™×•× ×¨×‘×™×¢×™, 30 במ××™ 2012 10:50:10 UTC+1, מ×ת Ian Collins:
You want

rv = BigNum_Initilaize(&a);
if(rv)
goto error_exit;
rv = BigNum_Add(&a,&b,&c);
if(!rv)
goto error_exit;

it's slightly messier than exceptions, but unlike exceptions it's easier to test and harder to abuse.

It's way uglier and no easier to test.

You don't want to be defining the symbol,ERROR_SUCCESS in a bignum.h header, and you don't want bignum.c dependent on myerrorcodes.h. However -1 for out of memory is pretty standard.

I'm not sure what you are saying there.

 

[Willem]

Malcolm McLean wrote:
) You want
)
) rv = BigNum_Initilaize(&a);
) if(rv)
) goto error_exit;
) rv = BigNum_Add(&a, &b, &c);
) if(!rv)
) goto error_exit;

Why one positive and one negative test? Is that a typo?


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT

 

[Stefan Ram]

be done to relieve this code duplication? It's ugly, it hurts my

The code below still has some duplication. I might write another
post later with less duplication. But the code below should be
structured and readable, but was not tested.

static ErrorCode MyFuncX
( BigNum const * const a,
BigNum const * const b,
BigNum const * const c,
BigNum const * const d )
{ return BigNum_Set( &b, 3 ); }

static ErrorCode MyFuncD
( BigNum const * const a,
BigNum const * const b,
BigNum const * const c )
{ BigNum d;
ErrorCode rv = BigNum_Initialize( &d );
if( rv == ERROR_SUCCESS )
{ rv = MyFuncX( a, b, c, &d ); BigNum_Free( &d ); }
return rv; }

static ErrorCode MyFuncC
( BigNum const * const a,
BigNum const * const b )
{ BigNum c;
ErrorCode rv = BigNum_Initialize( &c );
if( rv == ERROR_SUCCESS )
{ rv = MyFuncD( a, b, &c ); BigNum_Free( &c ); }
return rv; }

static ErrorCode MyFuncB( BigNum const * const a )
{ BigNum b;
ErrorCode rv = BigNum_Initialize( &b );
if( rv == ERROR_SUCCESS )
{ rv = MyFuncC( a, &b ); BigNum_Free( &b ); }
return rv; }

ErrorCode MyFuncA( void )
{ BigNum a;
ErrorCode rv = BigNum_Initialize( &a );
if( rv == ERROR_SUCCESS )
{ rv = MyFuncB( &a ); BigNum_Free( &a ); }
return rv; }

ErrorCode MyFunc( void ){ return MyFuncA(); }

 

[Guest]

בת×ריך ×™×•× ×¨×‘×™×¢×™, 30 במ××™ 2012 10:50:10 UTC+1, מ×ת Ian Collins:

I was tempted to say the same. Exceptiosn and RAII should be able to make it quite tidy.

You want

rv = BigNum_Initilaize(&a);
if(rv)
goto error_exit;
rv = BigNum_Add(&a, &b, &c);
if(!rv)
goto error_exit;

it's slightly messier than exceptions, but unlike exceptions it's easier to test and harder to abuse.
why?

You don't want to be defining the symbol,ERROR_SUCCESS in a bignum.h header,

why? It's part of the Bignum interface.

and you don't want bignum.c dependent on myerrorcodes.h.
ditto

However -1 for out of memory is pretty standard.

I'd rather use a named constant... (I only recently found out this was a controversial opinion!)

 

[Stefan Ram]

The code below still has some duplication. I might write another
post later with less duplication.

/* This general function can be put in a library */
static ErrorCode BigNum_New
( ErrorCode( *continuation )( void const * data, BigNum * const num ),
void * data )
{ BigNum n;
ErrorCode rv = BigNum_Initialize( &n );
if( rv == ERROR_SUCCESS )
{ rv = continuation( data, &n ); BigNum_Free( &n ); }
return rv; }

/* here the application code starts */
struct BigNum { BigNum * a; BigNum * b; BigNum * c; BigNum * d; };

static ErrorCode MyFuncX( void const * this )
{ return BigNum_Set( &((( struct BigNum * )this )->b ), 3 ); }

static ErrorCode MyFuncD( void const * this, BigNum * const num )
{ (( struct BigNum * )this )->d = num; return BigNum_New( MyFuncX, &this ); }

static ErrorCode MyFuncC( void const * this, BigNum * const num )
{ (( struct BigNum * )this )->c = num; return BigNum_New( MyFuncD, &this ); }

static ErrorCode MyFuncB( void const * this, BigNum * const num )
{ (( struct BigNum * )this )->b = num; return BigNum_New( MyFuncC, &this ); }

static ErrorCode MyFuncA( void const * this, BigNum * const num )
{ (( struct BigNum * )this )->a = num; return BigNum_New( MyFuncB, &this ); }

ErrorCode MyFunc( void )
{ struct BigNum this; return BigNum_New( MyFuncA, &this ); }

 

[tom st denis]

If you want to be more concise, you can make ERROR_SUCCESS equal 0,
and then you can simply do:

    rv = BigNum_Initialize(&a) || ... || BigNum_Set(&b, 3) || BigNum_AddTo(&a, &b) || ... ;

Which will short-circuit at the first failure.

Which in a sufficiently complicated function like an ECC point add/
double would result in spaghetti code and your termination.

The "if (err == OK) ..." method is cool but frankly I just like the
goto's when I'm writing out something the long way.

Tom

 

[BartC]

ErrorCode MyFunc()
{
BigNum a, b, c, d;
ErrorCode rv;

rv = BigNum_Initialize(&a);
if(rv != ERROR_SUCCESS)
return(rv);

But this is nasty, with all those duplicated "free" blocks. What can
be done to relieve this code duplication? It's ugly, it hurts my
freaking eyes and my nose is screaming for relief. And it hurts
maintainability to no end -- what if we decide to add more capability
to MyFunc() that requires additional bignums? Oy... We'd have to
update _all those blocks_.

I think you've designed in too much error-checking, and made your bignums
unwieldy to initialise and manage.

Any code trying to actually do something with bignums would be lost amongst
all the error-checking and recovery. And if there was some conditional code,
and every single calculation needed checking, then you would easily lose
track of what needed freeing and what didn't.

Why does an initialisation need error-checking anyway? Does it involve
allocating memory?

It's not clear if a bignum is a pointer or a struct; they could be simply
initialised to NULL or to {0}, leaving it to the bignum arithmetic routines
to allocate as needed, and do the error-checking. Your code might still need
to free, but the free routine can leave alone bignums that are still NULL or
{0}.

Actually, for memory management, you should take seriously the idea put
forward of using garbage collection. Then you don't need to explicitly free
memory.

ErrorCode MyFunc()
{
BigNum a, b, c, d;
ErrorCode rv;

rv = BigNum_Initialize(&a); if(rv != ERROR_SUCCESS) goto
fail0;
rv = BigNum_Initialize(&b); if(rv != ERROR_SUCCESS) goto
fail1;
rv = BigNum_Initialize(&c); if(rv != ERROR_SUCCESS) goto
fail2;
rv = BigNum_Initialize(&d); if(rv != ERROR_SUCCESS) goto
fail3;

(As a personal preference, I would use something like BN_Init(). Then these
big names would dominate the code so much.)

And, whatever the initialisation does, is it really necessary to check the
result? Why not leave it to routines such as BigNum_Set() to validate it's
arguments; if an argument has not been properly initialised, then *it*
returns a failure code.

/* do some math */
rv = BigNum_Set(&b, 3); if(rv != ERROR_SUCCESS) goto fail4;

...

/* free buffers */
rv = BigNum_Free(&d); if(rv != ERROR_SUCCESS) goto fail3;

And what is likely to go wrong with BigNum_Free() that will need checking?
Will the caller of this function care, provided it gets the right answer? Or
is the result of BigNum_Set() likely to be invalidated if a subsequent free
fails?

rv = BigNum_Free(&c); if(rv != ERROR_SUCCESS) goto fail2;
rv = BigNum_Free(&b); if(rv != ERROR_SUCCESS) goto fail1;
rv = BigNum_Free(&a); if(rv != ERROR_SUCCESS) goto fail0;

return(ERROR_SUCCESS);

fail4: BigNum_Free(&d);
fail3: BigNum_Free(&c);
fail2: BigNum_Free(&b);
fail1: BigNum_Free(&a);
fail0: return(rv);
}

If you do need to do this, then just combine all the statuses of multiple
calls to BigNum_Free() (the error code needs to be of a format that will
allow | or & operations).

I know your post is about managing error recovery, rather than the merits of
a particular library. But, someone needing to do arithmetic via function
calls, might prefer to keep error-checking low-key, perhaps something like
this:

ErrorCode BigNum_Average(BigNum A, BigNum B, BigNum Result) {
BigNum Two=NULL;
ErrorCode e=0;

e |= BigNum_Add(A,B, Result);
e |= BigNum_Set(&Two, 2);
e |= BigNum_Div(Result, Two, Result);

e |= BigNum_Free(&Two);

return e;
}

Then it is still reasonably easy to follow what's going on. (Actually, I
wouldn't bother with the error codes myself; I would build a status into the
bignum itself, a bit like the Nans of floating point.)

 

[Stefan Ram]

/* This general function can be put in a library */

The next variant is less clean/general due to object
identifiers with file scope, but the »application code«
now is as concise as possible.

/* This general function can be put away into a library */
ErrorCode BigNum_New
( ErrorCode( *continuation )( void ), BigNum * const n )
{ ErrorCode rv = BigNum_Initialize( &n );
if( rv == ERROR_SUCCESS )
{ rv = continuation(); BigNum_Free( &n ); }
return rv; }

/* here the application code starts */
static BigNum a; static BigNum b; static BigNum c; static BigNum d;
static ErrorCode MyFuncX( void ){ return BigNum_Set( &b, 3 ); }
static ErrorCode MyFuncD( void ){ return BigNum_New( MyFuncX, &d ); }
static ErrorCode MyFuncC( void ){ return BigNum_New( MyFuncD, &c ); }
static ErrorCode MyFuncB( void ){ return BigNum_New( MyFuncC, &b ); }
ErrorCode MyFunc( void ){ return BigNum_New( MyFuncB, &a ); }

 

[Stefan Ram]

( ErrorCode( *continuation )( void ), BigNum * const n )
{ ErrorCode rv = BigNum_Initialize( &n );
if( rv == ERROR_SUCCESS )
{ rv = continuation(); BigNum_Free( &n ); }

Oops, I think »&n« should become »n« above.

 

[Stefan Ram]

The next variant is less clean/general due to object
identifiers with file scope, but the »application code«
now is as concise as possible.

In this special case, it can be made a little more concise:

/* This general function can be put away into a library */
/* (unchanged since the last post, except for an error correction) */
ErrorCode BigNum_New
( ErrorCode( *continuation )( void ), BigNum * const n )
{ ErrorCode rv = BigNum_Initialize( n );
if( rv == ERROR_SUCCESS )
{ rv = continuation(); BigNum_Free( n ); }
return rv; }

/* here the application code starts */
static BigNum a[ 4 ]; static int i; static ErrorCode MyFuncX()
{ return i < 4 ? BigNum_New( MyFuncX, a + i++ ): BigNum_Set( a + 1, 3 ); }
ErrorCode MyFunc( void ){ i = 0; return MyFuncX(); }

 

[Ike Naar]

I was thinking about using a "goto", but Gotos are Bad, aren't they? I
can't believe I may have to use a goto! What about Djikstra's famous
letter? Is it possible that perhaps a goto may be GOOD here? Because
with a goto this would all seem much simpler. If we violate Djikstra,
we get:

The name is Dijkstra, not Djikstra.