2

Go through the following C code

# define swap(a,b) temp=a; a=b; b=temp;
main( )
{
    int i, j, temp;
    i=5;
    j=10;
    temp=0;
    if( i > j)
        swap ( i, j );
    printf ( "%d %d %d", i, j, temp);
}

Compiler Output:

10 0 0

I am Expecting this output

10 5 0

Why am I wrong??

Megamind
  • 41
  • 4

6 Answers6

5

It's the lack of braces. This is one of the common pitfalls with macros. Let's see what happens:

if(i > j)
    swap(i, j);

becomes:

if(i > j)
    temp = a; a = b; b = temp;;

Made a little more readable:

if(i > j)
    temp = a;
a = b;
b = temp;

So the lines a = b; and b = temp; will always be executed, they fall outside the if body.

Either put braces around the if, or the macro.

Kninnug
  • 7,992
  • 1
  • 30
  • 42
5

It is much better to define statement-like macros with a do{...}while(0) like (see this):

#define swap(a,b) do { int temp=a; a=b; b=temp; } while(0)

since the do{....}while(0) construct is correctly understood (even as the then or else part of a conditional, etc. etc... as explained in other replies.

Actually, you'll still have issues if you invoke swap with a temp argument (e.g. swap(temp,xx)) or if you use it with a side effecting second argument (e.g. swap(x,y++) would increment y twice and not do what you want).

A pedantically robust swap might use concatenation at preprocessing level and some unique counter like the GCC specific __COUNTER__ predefined macro to generate a unique symbol (instead of temp)

Community
  • 1
  • 1
Basile Starynkevitch
  • 223,805
  • 18
  • 296
  • 547
2

Your problem is the if condition in this line:

if( i > j)
swap ( i, j );

After the preprocessor has replaced your define, these lines look like the following:

if( i > j)
temp=a; a=b; b=temp;

What you expect is the following:

if( i > j)
{temp=a; a=b; b=temp;}

which can be achieved by changing your define to 

#define swap(a,b) {temp=a; a=b; b=temp;}

EDIT: As mentioned by others, a more robust solution would be to define the macro as

#define swap(a,b) do{temp=a; a=b; b=temp;}while(1)

This still requires the variable temp to be declared by hand.

A good example of a type independent swap macro can be found here: https://stackoverflow.com/a/3982430/5237890

Community
  • 1
  • 1
JanK
  • 160
  • 6
  • 1
    Still won't change the output since i is not greater than j – FredK Aug 24 '15 at 16:07
  • This is true but questioner expected the values not to be swapped. – JanK Aug 24 '15 at 16:27
  • Just braces `{}` is not enough, because something like `if (x) swap(a,b); else something();` will not work. Basile's answer gives the trick that is commonly used to avoid this kind of problem. – Jens Gustedt Aug 24 '15 at 19:47
2

You should add curly braces to your macro declaration to solve your problem.

Moreover, if you want to swap basic type, like int for example, you can use the XOR operator to swap the values without use an additional variable.

#define swap(a, b) { a ^= b; b ^= a; a ^= b; }
Paul Boutes
  • 3,285
  • 2
  • 19
  • 22
1

Another problem is the line

if( i > j)

In your case, i=5 and j=10, so the first statement of theswap() line is not executed. As @Kninnug pointed out (almost), without curly braces your code is actually

if(i > j)
    temp = i;
i = j
j = temp;

Now, since I=5 and j=10, only the last two lines are executed, so I becomes 10 (the value of j) and j becomes zero (the value of temp).

FredK
  • 4,094
  • 1
  • 9
  • 11
0

When using a function-like macro the compiler just replaces the text in the code where you call the function with the text in your macro declaration. You did not use curly braces after the if statement which would be fine if you only needed to execute one line of code. But when the compiler does the text replacement for your macro function, there are now three lines of code that need to be nested under the if statement.

Adding curly braces around the line Swap ( a, b ) will fix your problem. Alternatively you can place curly braces around the three lines of code in your macro declaration.

Dsel
  • 1,027
  • 3
  • 13
  • 22