Sign in to follow this  
Wavesonics

cmath::fabs() VS my macro. Different result.

Recommended Posts

Wavesonics    330
Ok so I wrote some code that deals with 3D bounding boxes which is not important. It was all working, and I later adapted it to a new project. In this project I have been dealing with many situations where i needed the absolute value of floats. So I wrote the following macro: #define ABS(i) ( i > 0 ? i : ( i*-1 ) ) and called it doing something like: ABS( fSomeFloat ) And life was good. So when I adapted this code i changes my calls for fabs() (which is the cmath float version of abs() which is of course, absolute value) to use my new macro ABS(). And things started going funny, so I ended up changing back to fabs() out of desperation and low and behold, it works perfectly again. WHY?! Is my macro wrong? (it must be) what does fabs() do differently?! Any ideas?

Share this post


Link to post
Share on other sites
Wavesonics    330
Won't that give you the absolute value of 1 (1) and then increment i to 2?

And sorry, just realized I posted this in Game Programing, I meant to do General Programing.

Share this post


Link to post
Share on other sites
Guest Anonymous Poster   
Guest Anonymous Poster
int i = 1;
ABS( ++i );

equals to:

int i = 1;
( ++i > 0 ? ++i : ( ++i*-1 ) );

Do you see the problem now ?

It will return 3! and i will be 3 too.

Share this post


Link to post
Share on other sites
Wavesonics    330
Yes but Im simply sending in the value of i, not i++.

Unless there is something Im missing on how macros work?

That Macro should simply evaluate to what ever the ABS is, not affect the value of i at all correct?

Share this post


Link to post
Share on other sites
Wavesonics    330
hhhmmm... you guys must be right.

I understand what you are saying,

my actual code looks like:

fabs((float)(curNode->min[0]) - pos.x)

or

ABS((float)(curNode->min[0]) - pos.x)

obviously.

I bet what is happening is it's taking the macro:
#define ABS(i) ( i > 0 ? i : ( i*-1 ) )

and in the false state it is doing:

( i*-1 ) = ( (float)(curNode->min[0]) - pos.x * -1 )

So order of operations, pos.x is being * -1 and THEN subtracted, thanks for the help guys! Seriously!

++rating for both ya ;)

Share this post


Link to post
Share on other sites
Ezbez    1164
This is reason #27 to not use a macro....

Seriously, just write an inlined function if you want to. It doesn't have to be ++i that you do in the call to ABS(), it could be any function, or pretty much anything. Besides, why don't you want to use fabs()? It's a good function!

Edit: Same time posting. What you want to do is wrap i inside of parenthesis. Order of operations means that it's only multiplying pos.x by -1. Like so:

(float)(curNode->min[0]) - pos.x*-1

Now, obviously, curNode->min[0] isn't being multiplied by -1! The horror!

Edit again: And it looks like you've figured it out, anyways.

Share this post


Link to post
Share on other sites
scorpion007    118
Quote:
Original post by Anonymous Poster
int i = 1;
ABS( ++i );

equals to:

int i = 1;
( ++i > 0 ? ++i : ( ++i*-1 ) );

Do you see the problem now ?

It will return 3! and i will be 3 too.


No, there is no guarantee that will return 3. It is undefined behavior. It might return anything.

You are modifying the variable i twice between 2 sequence points.

More info on sequence points

Share this post


Link to post
Share on other sites
PlayerX    279
This is why often macros are defined using a liberal sprinkling of parentheses anywhere the arguments are used, eg:

#define ABS(i) ( (i) > 0 ? (i) : ( (i) * -1 ) )

This won't stop the pre-/post-increment problem, but it solves most of the other side effects.

Share this post


Link to post
Share on other sites
Washu    7829
Macros are a text substitution during the preprocessing stage of compilation. The Macro identifiers are literally replaced with the definition. As has been pointed out, expressions like M(++i) or M(i++) or M(i*i) can often times result in unexpected results when M is a macro.

You should avoid macros as much as possible. I've found that the only areas where I used them are for inclusion guards and debugging assertions. All other uses are suspect and should be carefully examined for validity.

Share this post


Link to post
Share on other sites
deffer    754
Quote:
Original post by scorpion007
Quote:
Original post by Anonymous Poster
int i = 1;
ABS( ++i );

equals to:

int i = 1;
( ++i > 0 ? ++i : ( ++i*-1 ) );

Do you see the problem now ?

It will return 3! and i will be 3 too.


No, there is no guarantee that will return 3. It is undefined behavior. It might return anything.

You are modifying the variable i twice between 2 sequence points.

More info on sequence points


Maybe I'm missing something, but ?: operator is short-circuit, so it has to evaluate it's arguments in specific order. This is not an undefined behaviour.

For example, this is fully defined:
(++i && --i || ++i)

And also this:
if (++i > 0) return ++i;
else return ++i*-1;


Of course, if someone really wants to use such complicated expressions. [smile]

Share this post


Link to post
Share on other sites
scorpion007    118
Quote:

For example, this is fully defined:
(++i && --i || ++i)

That's because && and || mark the end of a sequence point. The : operator in the ternary expression does not. Only the first operand is guaranteed to be executed before the other two (before the ?).

Quote:

And also this:
if (++i > 0) return ++i;
else return ++i*-1;


Of course, if someone really wants to use such complicated expressions. [smile]


The expression enclosed inside the if () is a `full expression', hence it will be executed before the return ++i;

The example posted by Anonymous Poster is indeed undefined.

Share this post


Link to post
Share on other sites
Vorpy    869
I think the example posted by the AP is not undefined. The expression before the ? is executed first, and of the expressions separated by the :, only one of them is executed. There is nothing undefined there.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this