CODEWITHSUNDEEP
c++macros
shek
shek

Reputation: 215

C++ macro (min max) not working properly

Why doesn't the following code work? It prints INT_MAX. But if I uncomment the two lines in the inner for loop, then it works fine (prints 2). I can't combine the two macros like that? Not sure if further detail is needed...pretty self explanatory.

Thanks.

#include <iostream>
#include <limits.h>

using namespace std;

#define min(a,b) a<b?a:b
#define max(a,b) a>b?a:b

int main(int argc, char **argv)
{
    int N = 100;
    int *drop = new int[N+1];
    drop[0] = 0; drop[1] = 1; drop[2] = 1;
    for(int i=3; i<=N; i++)
    {
        drop[i] = INT_MAX;
        for(int start=1; start<=i; start++)
        {
            drop[i] = min(drop[i], max(start, drop[i-start]+1));
            //int x = max(start, drop[i-start]+1);
            //drop[i] = min(drop[i], x);
        }
    }
    cout<<drop[3]<<endl;
    return 0;
}

Upvotes: 3

Views: 14831

Answers (3)

Shreevardhan
Shreevardhan

Reputation: 12641

C++ already has std::min and std::max defined in <algorithm>. You can change your code to a pure C++ version

#include <iostream>
#include <algorithm>
#include <limits>
using namespace std;

int main(int argc, char ** argv) {
    int N = 100;
    int * drop = new int[N + 1];
    drop[0] = 0;
    drop[1] = drop[2] = 1;
    for (int i = 3; i <= N; ++i) {
        drop[i] = numeric_limits<int>::max();    // <limits>
        for(int start = 1; start <= i; ++start)
            drop[i] = min(drop[i], max(start, drop[i - start] + 1));    // <algorithm>
    }
    cout << drop[3] << endl;
    return 0;
}

Upvotes: 9

Michael
Michael

Reputation: 5897

Rather than an answer, it's a plead to all developers out there: please don't use macros like that. C++ offers template functions for these purposes. Remember that macros just substitute parameters rather than pre-evaluating them. Even if you add parentheses as samgak explained, this only fixes a half of the problem. Consider code like this:

int x = 5;
int y = max(++x, 0);

The caller would expect x=6 and y=6 after that; however the macro will get expended into

int y = (++x > 0)? ++x : 0;

causing x=7 and y=7.

Upvotes: 8

samgak
samgak

Reputation: 24417

Put brackets around the terms in your macros:

#define min(a,b) ((a)<(b)?(a):(b))
#define max(a,b) ((a)>(b)?(a):(b))

As it is, this:

drop[i] = min(drop[i], max(start, drop[i-start]+1));

is expanding to this (without brackets):

drop[i] < start > drop[i-start]+1 ? start: drop[i-start]+1 ? drop[i] : start > drop[i-start]+1 ? start: drop[i-start]+1;

which may not evaluate in the order you intend. Using brackets enforces the correct order of operations.

As noted in the comments, you shouldn't use macros with expressions that have side effects if the macro arguments are evaluated more than once.

Upvotes: 12

Related Questions