1

reading combase.cpp code, I find following:

/* We have to ensure that we DON'T use a max macro, since these will typically   */                    
/* lead to one of the parameters being evaluated twice.  Since we are worried    */                    
/* about concurrency, we can't afford to access the m_cRef twice since we can't  */                    
/* afford to run the risk that its value having changed between accesses.        */                    

    template<class T> inline static T ourmax( const T & a, const T & b )                                   
    {
        return a > b ? a : b;       
    } 

I don't understand why "max macro leads to one of the parameters being evaluated twice"?

Mat
  • 202,337
  • 40
  • 393
  • 406
  • See the answers to [this question](http://stackoverflow.com/questions/5323733/why-such-macro-in-c). – DCoder May 22 '12 at 05:04

2 Answers2

2

Consider an usage like in this code sample:

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

int main()
{

  int a = 0;
  int b = 1;

  int c = max(a++, b++);

  cout << a << endl << b << endl;
  return 0;

}

The intention probably was to print 1 and 2, but macro expands to:

int c = a++ > b++ ? a++ : b++;

b gets incremented twice, and the program prints 1 and 3.

Hence,
In some cases, expressions passed as arguments to macros can be evaluated more than once.

Alok Save
  • 202,538
  • 53
  • 430
  • 533
  • The priority of > sign in the max macro can be lowered by the arguments passed in. `#define max(a, b) ((a) > (b) ? (a) : (b))` can avoid such thing from happening. – nhahtdh May 22 '12 at 05:11
  • @nhahtdh That solves the priority issues (which weren't a problem in Als example), but still leaves one of the parameters evaluated twice. – James Kanze May 22 '12 at 07:51
  • I just want to comment on the precedence issue. – nhahtdh May 22 '12 at 08:00
1

Although Als has quite clearly explained the immediate issue, I see two larger issues. The first is simple: max can't be a macro, since it is a standard function template, defined in <algorithm>. (In the case of VC++, you need to define NOMINMAX in order to use <algorithm>. But since it's always preferable to use a standard function when it does the job, you should practically always add NOMINMAX to your preprocessor defines, and be done with it.)

The second is even more worrisome, since it shows a lack of understanding concerning the code. The comments make reference to "concurrency", and suggest that by using the function, there are no concurrency issues. This is simply incorrect. If any other thread (or process, in the case of shared memory) may modify either of the arguments, the behavior is undefined. In particular, as written, the compiler likely would read one of the values twice; the arguments are references. But regardless of how you write it, the compiler is allowed to reread the values; and even if it doesn't, there's nothing to ensure that the accesses are atomic. Whoever wrote the comment does not understand the basic principles of multithreaded code.

James Kanze
  • 150,581
  • 18
  • 184
  • 329