Nested ifs and speed.

C

charlie

I recently came across s function (actually many) whose structure is a
series of nested ifs with the meat of the function at the very centre:

if (condition1)
if (condition2)
if (condition3)
if (condition4)
<Body>
else {
printf("Condition4 failed");
return;
}
else {
printf("Condition3 failed");
return;
}
else {
printf("Condition2 failed");
return;
}
else {
printf("Condition1 failed");
return;
}

To me this is really unreadable and I would re-write as:

if(!condition1) {
printf("Condition 1 failed");
return;
}

....
<Body>


I got to thinking why would you write code this way; seperating the
condition from it's consequences. It occured to me that there may have
been performance related concerns with respect to jumps so with the
original code, in the case of all conditions suceeding, you get no
jumps but with my code you may get as many jumps as there are
conditions because each if block would need to be skipped. I am
certainly not an expert in compilers or machine code and this is
probably a case of premature optimization in either case I just trying
to get into the head of the guy that wrote it in the first place.

Cheers,
Charlie.
 
R

Richard Bos

charlie said:
I recently came across s function (actually many) whose structure is a
series of nested ifs with the meat of the function at the very centre:

if (condition1)
if (condition2)
if (condition3)
if (condition4)
<Body>
else {
printf("Condition4 failed");
return;
}
else {
printf("Condition3 failed");
return;
}
else {
printf("Condition2 failed");
return;
}
else {
printf("Condition1 failed");
return;
}

To me this is really unreadable and I would re-write as:

if(!condition1) {
printf("Condition 1 failed");
return;
}

...
<Body>

I got to thinking why would you write code this way; seperating the
condition from it's consequences. It occured to me that there may have
been performance related concerns with respect to jumps
Click to expand...

Nah. It's probably just another Wirthian with blinkers on: One Entry,
One Exit, Or Else!

Richard
 
C

charlie

Richard said:
Nah. It's probably just another Wirthian with blinkers on: One Entry,
One Exit, Or Else!

Richard
Click to expand...

Actually, I just looked at the code again and saw that the else
branches do not contain return statement so you may well be correct:

if (condition1)
if (condition2)
if (condition3)
if (condition4)
<Body>
else {
printf("Condition4 failed");
}
else {
printf("Condition3 failed");
}
else {
printf("Condition2 failed");
}
else {
printf("Condition1 failed");
}
 
C

Chris Dollin

charlie said:
Actually, I just looked at the code again and saw that the else
branches do not contain return statement so you may well be correct:

if (condition1)
if (condition2)
if (condition3)
if (condition4)
<Body>
else {
printf("Condition4 failed");
}
else {
printf("Condition3 failed");
}
else {
printf("Condition2 failed");
}
else {
printf("Condition1 failed");
}
Click to expand...

But that code could be written with closer association as

if (!c1) printf( "blah");
else if (!c2) printf( "bleh" );
else if (!c3) printf( "blih" );
else if (!c4) printf( "bloh" );
else <body>

(layout and braces to taste), so the original layout doesn't
suggest having to stick to the one-exit style.

[Should there be a \n at the end of all these printfs?]
 
G

Giorgio Silvestri

Chris Dollin said:
But that code could be written with closer association as

if (!c1) printf( "blah");
else if (!c2) printf( "bleh" );
else if (!c3) printf( "blih" );
else if (!c4) printf( "bloh" );
else <body>
Click to expand...

Something like this ?

#define NC 2

static void (*condition[NC][NC][NC][NC])(void) = {
/* functions ... */
};


(*condition[!!(c1)][!!(c2)][!!(c3)][!!(c4)])();

or

(*condition[c1][c2][c3][c4])();

if c<i> is 0 o 1.

I don't want to be time/space saving, just another idea.

Of course for a small number of conditions!
Observe that in this case ALL conditions are calculated!
 
B

BubbaGump

I recently came across s function (actually many) whose structure is a
series of nested ifs with the meat of the function at the very centre:

if (condition1)
if (condition2)
if (condition3)
if (condition4)
<Body>
else {
printf("Condition4 failed");
return;
}
else {
printf("Condition3 failed");
return;
}
else {
printf("Condition2 failed");
return;
}
else {
printf("Condition1 failed");
return;
}

To me this is really unreadable and I would re-write as:

if(!condition1) {
printf("Condition 1 failed");
return;
}

...
<Body>


I got to thinking why would you write code this way; seperating the
condition from it's consequences. It occured to me that there may have
been performance related concerns with respect to jumps so with the
original code, in the case of all conditions suceeding, you get no
jumps but with my code you may get as many jumps as there are
conditions because each if block would need to be skipped. I am
certainly not an expert in compilers or machine code and this is
probably a case of premature optimization in either case I just trying
to get into the head of the guy that wrote it in the first place.

Cheers,
Charlie.
Click to expand...


One way to allocate and free some set of resources without retyping
the freeing code multiple times is nested ifs.

#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a, *b, *c;

a = malloc(1);
if(!a)
{
printf("allocation of a failed\n");
}
else
{
b = malloc(1);
if(!b)
{
printf("allocation of b failed\n");
}
else
{
c = malloc(1);
if(!c)
{
printf("allocation of c failed\n");
}
else
{
/* todo: do some operation which involves
* a, b, and c here */

free(c);
}

free(b);
}

free(a);
}

/* todo: set value based on success or failure */
return 0;
}


Another way is a goto with a cleanup label at the end of the function.


#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a = NULL, *b = NULL, *c = NULL;

a = malloc(1);
if(!a)
{
printf("allocation of a failed\n");
goto cleanup;
}

b = malloc(1);
if(!b)
{
printf("allocation of b failed\n");
goto cleanup;
}

c = malloc(1);
if(!c)
{
printf("allocation of c failed\n");
goto cleanup;
}

/* todo: do some operation which involves
* a, b, and c here */

cleanup:

if(a)
{
free(a);
}

if(b)
{
free(b);
}

if(c)
{
free(c);
}

/* todo: set value based on success or failure */
return 0;
}


Another way is a goto with a series of cleanup labels at the end of
the function, one for each resource.


#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a, *b, *c;

a = malloc(1);
if(!a)
{
printf("allocation of a failed\n");
goto cleanup_a;
}

b = malloc(1);
if(!b)
{
printf("allocation of b failed\n");
goto cleanup_b;
}

c = malloc(1);
if(!c)
{
printf("allocation of c failed\n");
goto cleanup_c;
}

/* todo: do some operation which involves
* a, b, and c here */

free(c);

cleanup_c:
free(b);

cleanup_b:
free(a);

cleanup_a:

/* todo: set value based on success or failure */
return 0;
}


If this is actually what you were asking about (?) then I think it's
only a style issue. No case should make much of a performance
difference. The middle one might be a few fractions of a percent
slower because of the extra ifs, but they should be quick compared to
the allocations. If the function really needed to be in a performance
critical path then it shouldn't even be doing the allocations anyway
(they should be done ahead of time somewhere else).
 
W

websnarf

charlie said:
I recently came across s function (actually many) whose structure is a
series of nested ifs with the meat of the function at the very centre:

if (condition1)
if (condition2)
if (condition3)
if (condition4)
<Body>
else {
printf("Condition4 failed");
return;
}
else {
printf("Condition3 failed");
return;
}
else {
printf("Condition2 failed");
return;
}
else {
printf("Condition1 failed");
return;
}

To me this is really unreadable and I would re-write as:

if(!condition1) {
printf("Condition 1 failed");
return;
}

...
<Body>


I got to thinking why would you write code this way; seperating the
condition from it's consequences. It occured to me that there may have
been performance related concerns with respect to jumps so with the
original code, in the case of all conditions suceeding, you get no
jumps
Click to expand...

You have to jump at least once to get out from inside the body to
rejoin the main flow.
[...] but with my code you may get as many jumps as there are
conditions because each if block would need to be skipped. I am
certainly not an expert in compilers or machine code and this is
probably a case of premature optimization in either case I just trying
to get into the head of the guy that wrote it in the first place.
Click to expand...

Well, if you have not determined that this is any important loop, then
it is a case of premature optimization. However, if you have
established that this is an important inner loop case, then it is not
premature. Very few programmers correctly comprehend the phrase
"premature optimization is the root of all evil". In particular most
cannot understand when the condition of "premature" has been
invalidated.

In any event, for clarity, there are two main categories for what is
going on here -- your inner loop and your errors. For clarity reasons,
this is how I would do it:

if (condition1 && condition2 && condition3 && condition4) {
/* No errors */
<Body>
} else {
/* Errors */
if (!condition1) printf("Condition1 failed");
/* else */ if (!condition2) printf("Condition2 failed");
/* else */ if (!condition3) printf("Condition3 failed");
/* else */ if (!condition4) printf("Condition4 failed");
return;
}

This is probably a lot easier to maintain.

For performance, there may be a question of accelerating: (condition1
&& condition2 && condition3 && condition4). If you expect all the
conditions to be true and equal to the value 1 (or have some shared
bit) most of the time, then you can change the && to a &, and this may
decrease the number instructions until you reach the body.

If you expect some of the conditions to fail, then leave the &&
operation, but the *order* in which you evaluate them will matter.
I.e., you want the condition to fail as quickly as possible, since the
order won't matter on success. So you put the conditions in decreasing
order of probability of failure. OTOH, the likelihood that this will
matter is basically 0, since you are eating penality for executing
"printf"'s on failure anyways.
 
B

BubbaGump

charlie said:
I recently came across s function (actually many) whose structure is a
series of nested ifs with the meat of the function at the very centre:

if (condition1)
if (condition2)
if (condition3)
if (condition4)
<Body>
else {
printf("Condition4 failed");
return;
}
else {
printf("Condition3 failed");
return;
}
else {
printf("Condition2 failed");
return;
}
else {
printf("Condition1 failed");
return;
}

To me this is really unreadable and I would re-write as:

if(!condition1) {
printf("Condition 1 failed");
return;
}

...
<Body>


I got to thinking why would you write code this way; seperating the
condition from it's consequences. It occured to me that there may have
been performance related concerns with respect to jumps so with the
original code, in the case of all conditions suceeding, you get no
jumps
Click to expand...

You have to jump at least once to get out from inside the body to
rejoin the main flow.
[...] but with my code you may get as many jumps as there are
conditions because each if block would need to be skipped. I am
certainly not an expert in compilers or machine code and this is
probably a case of premature optimization in either case I just trying
to get into the head of the guy that wrote it in the first place.
Click to expand...

Well, if you have not determined that this is any important loop, then
it is a case of premature optimization. However, if you have
established that this is an important inner loop case, then it is not
premature. Very few programmers correctly comprehend the phrase
"premature optimization is the root of all evil". In particular most
cannot understand when the condition of "premature" has been
invalidated.

In any event, for clarity, there are two main categories for what is
going on here -- your inner loop and your errors. For clarity reasons,
this is how I would do it:

if (condition1 && condition2 && condition3 && condition4) {
/* No errors */
<Body>
} else {
/* Errors */
if (!condition1) printf("Condition1 failed");
/* else */ if (!condition2) printf("Condition2 failed");
/* else */ if (!condition3) printf("Condition3 failed");
/* else */ if (!condition4) printf("Condition4 failed");
return;
}

This is probably a lot easier to maintain.
Click to expand...


That's not realistic. Each of those conditions is likely to be
calculated from the result of some function and also to have
calculations interspersed between it and the next condition, which
means not fitting nicely (or at all) into a single if().
 
C

charlie

BubbaGump said:
One way to allocate and free some set of resources without retyping
the freeing code multiple times is nested ifs.

#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a, *b, *c;

a = malloc(1);
if(!a)
{
printf("allocation of a failed\n");
}
else
{
b = malloc(1);
if(!b)
{
printf("allocation of b failed\n");
}
else
{
c = malloc(1);
if(!c)
{
printf("allocation of c failed\n");
}
else
{
/* todo: do some operation which involves
* a, b, and c here */

free(c);
}

free(b);
}

free(a);
}

/* todo: set value based on success or failure */
return 0;
}


Another way is a goto with a cleanup label at the end of the function.


#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a = NULL, *b = NULL, *c = NULL;

a = malloc(1);
if(!a)
{
printf("allocation of a failed\n");
goto cleanup;
}

b = malloc(1);
if(!b)
{
printf("allocation of b failed\n");
goto cleanup;
}

c = malloc(1);
if(!c)
{
printf("allocation of c failed\n");
goto cleanup;
}

/* todo: do some operation which involves
* a, b, and c here */

cleanup:

if(a)
{
free(a);
}

if(b)
{
free(b);
}

if(c)
{
free(c);
}

/* todo: set value based on success or failure */
return 0;
}


Another way is a goto with a series of cleanup labels at the end of
the function, one for each resource.


#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a, *b, *c;

a = malloc(1);
if(!a)
{
printf("allocation of a failed\n");
goto cleanup_a;
}

b = malloc(1);
if(!b)
{
printf("allocation of b failed\n");
goto cleanup_b;
}

c = malloc(1);
if(!c)
{
printf("allocation of c failed\n");
goto cleanup_c;
}

/* todo: do some operation which involves
* a, b, and c here */

free(c);

cleanup_c:
free(b);

cleanup_b:
free(a);

cleanup_a:

/* todo: set value based on success or failure */
return 0;
}


If this is actually what you were asking about (?) then I think it's
only a style issue. No case should make much of a performance
difference. The middle one might be a few fractions of a percent
slower because of the extra ifs, but they should be quick compared to
the allocations. If the function really needed to be in a performance
critical path then it shouldn't even be doing the allocations anyway
(they should be done ahead of time somewhere else).
Click to expand...

It is actually not mallocs but fopens but the principle is pretty much
the same. I have looked at more of the code now and I've come to the
conclusion that readability was not a priority. There are several >
1000 line procedures and nearly no comments. Thank-you (all) for your
suggestions on how to make it more readable and sensible. I will put
them into practice when I have introduced the code to the concept of
abstraction (and other coding practices too). I just want to be able to
read and debug the code without feeling like I need to delete the whole
shebang and start again, which will never happen of course.
Frustrated coders of the world unite!

Cheers
Charlie.
 
C

Christopher Benson-Manica

charlie said:
I recently came across s function (actually many) whose structure is a
series of nested ifs with the meat of the function at the very centre:
Click to expand...

(code snipped)
To me this is really unreadable and I would re-write as:
Click to expand...
if(!condition1) {
printf("Condition 1 failed");
return;
}
Click to expand...
...
<Body>
Click to expand...

Alternately, you could abuse the preprocessor a bit:

#define VALIDATE(c) (c?1:(printf("Condition "#c" failed\n"),0))

if( VALIDATE(condition1) && VALIDATE(condition2) && ... ) {
<body>
}

(I make no representations about whether this would be advisable.
Notice that you end up with an additional conditional check for every
condition, which may not always be neglible.)
 
M

Michael Wojcik

One way to allocate and free some set of resources without retyping
the freeing code multiple times is nested ifs. [...]

Another way is a goto with a cleanup label at the end of the function.

#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a = NULL, *b = NULL, *c = NULL;

[...]

cleanup:

if(a)
{
free(a);
}

if(b)
{
free(b);
}

if(c)
{
free(c);
}
Click to expand...

free(NULL) is guaranteed safe. You can replace this fragment with:

cleanup:
free(a);
free(b);
free(c);

which takes far less vertical space, allowing the reader to see more
of the surrounding function.

Of course, the same is not true of many other cleanup functions,
such as fclose (for some inexplicable reason). For the specific
case of cleanup code at the end of a function, though, I personally
don't mind an abbreviated style, as in:

cleanup:
free(a);
if (b) fclose(b);
free(c);

which ought to be perfectly clear to the reader - assuming that
the "cleanup" section of code in fact just performs cleanup.
 
B

BubbaGump

One way to allocate and free some set of resources without retyping
the freeing code multiple times is nested ifs. [...]

Another way is a goto with a cleanup label at the end of the function.

#include <stdio.h>
#include <stdlib.h>

int main()
{
void *a = NULL, *b = NULL, *c = NULL;

[...]

cleanup:

if(a)
{
free(a);
}

if(b)
{
free(b);
}

if(c)
{
free(c);
}
Click to expand...

free(NULL) is guaranteed safe. You can replace this fragment with:

cleanup:
free(a);
free(b);
free(c);

which takes far less vertical space, allowing the reader to see more
of the surrounding function.

Of course, the same is not true of many other cleanup functions,
such as fclose (for some inexplicable reason). For the specific
case of cleanup code at the end of a function, though, I personally
don't mind an abbreviated style, as in:

cleanup:
free(a);
if (b) fclose(b);
free(c);

which ought to be perfectly clear to the reader - assuming that
the "cleanup" section of code in fact just performs cleanup.
Click to expand...

I like it.
 

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. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

Linux: using "clone3" and "waitid" 0
Order of evaluation in conditionals 3
Boomer trying to learn coding in C and C++ 6
nested structures and initialization 8
Looking for someone to take alook at this code and help 2
Fading effect between play and play-over and pause and pause-over 0
Rock, Paper, Scissor game. Im getting TypeError, unsupported operand type(s) for -=: 'NoneType' and 'int' 2
nested dictionaries and functions in data structures. 0

Members online

No members online now.
Total: 81 (members: 1, guests: 80)
Robots: 78

Forum statistics

Threads
473,999
Messages
2,570,247
Members
46,844
Latest member
JudyGvh32

Latest Threads

Top