We figured they were more actual guidelines. -- Pirates of the Caribbean

Macros are wonderful things.  I love macros.  They're exciting and dangerous.  Every time I write a macro I salivate at the power I am wielding.  It's like using a blowtorch.

It is perhaps no coincidence that many coding standards put big warning signs around macroland.  "Consider carefully before using macros", some say. "When considering using a macro, see if you can use an inline function instead," some say.  "Macros are frequent sources of bugs," some say.  "C++ has evolved to the point where, with inline functions and templates, macros are no longer necessary".

Heresy.

All absolutely true, and yet heresy nonetheless.  It is true that there are many bugs out there due to misuse of macros.  And yet only a few simple tips are necessary to prevent misuse of macros: (The problem is that the tips don't always get followed).

She generally gave herself very good advice, (though she very seldom followed it) -- Alice in Wonderland

1. By convention, name macros in ALL_CAPS.  This makes it easy to recognize them.
   Without this rule it is easy to think that the following macro is a function...
   #define SafeDelete(p) { delete (p); (p) = NULL; } void(0)
   ... and in fact it probably should be...
   template<typename T> SafeDelete(T *&p) { delete p; p = NULL; }
   
2. Parenthesize the definition of the macro.
   This prevents precedence bugs.
   #define SQUARE(x) (x) * (x) // should be ((x) * (x))
   int four = 16 / SQUARE(2); // BUG: four == 16
   
3. Parenthesize uses of the arguments of the macro unless this contravenes the purpose of the macro.
   This prevents precedence bugs in the other direction.
   #define SQUARE(x) (x * x) // should be ((x) * (x))
   int four = SQUARE(1 + 1); // BUG: four == 3
   
4. Fill out optional clauses.
   #define ASSERT(x) if (!(x)) { exit(__LINE__); }
   // should be if (!(x)) { exit(__LINE__); } else {} (void)0
   if (IsTuesday())
       ASSERT(m_pGarbage);
   else { // BUG: will only vacuum and check mail on tuesday (and only if m_pGarbage is set)
       Vacuum();
       CheckMail();
   }
   
5. UNDER NO CIRCUMSTANCES use an expression that has side effects as an argument when calling a macro.
   #define SafeDelete(p) { delete (p); (p) = NULL; } void(0) // reasonable, except for ALL_CAPS rule
   // p points to the first of a null-terminated array of pointers that need to be deleted
   while (p) SafeDelete(p++); // BUG: only odd-numbered pointers deleted, and AV if odd number of elements
   
   Exceptions I happen to agree with: VERIFY(...) cries out to be an exception to this rule since it commonly textifies the condition, and a popup that says "CloseHandle(m_file) failed" is infinitely more meaningful than "bOK failed".  As a corollary, VERIFY(...) macros should only evaluate their arguments once.
   Exceptions I've given up trying to convince other people are bad: SUCCEEDED(...), FAILED(...), and HRESULT_FROM_WIN32(GetLastError()) are common exceptions too.

Let's talk about the "no expressions with side effects" rule a little more.  It is OK to do things like

if (SUCCEEDED(CoInitialize(...))) { ... }

if (FAILED(CoCreateInstance(...))) { ... }

return HRESULT_FROM_WIN32(GetLastError());

... but for different reasons.  SUCCEEDED(...) and FAILED(...) only evaluate their arguments once, as can be verified from their definitions in winerror.h.  I'm using the Vista RTM version of winerror.h:

//
// Generic test for success on any status value (non-negative numbers
// indicate success).
//

#define SUCCEEDED(hr) (((HRESULT)(hr)) >= 0)

//
// and the inverse
//

#define FAILED(hr) (((HRESULT)(hr)) < 0)

Note the heavy (and correct) use of parentheses.  I still personally don't think it's a good idea to rely on this, since macros have a habit of changing... but I won't ding anyone on a code review for it.

HRESULT_FROM_WIN32(GetLastError()) is OK for a totally different reason.  HRESULT_FROM_WIN32(...), classically, does evaluate its argument multiple times... this time I'm using the Windows 2000 version of winerror.h:

#define HRESULT_FROM_WIN32(x)   (x ? ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)) : 0 )

... so this looks like a bug... until you realize that GetLastError() is idempotent.  If you call it a bunch of times in a row on the same thread, it will (per its contract) always return the same value.

Note that the macro is not completely parenthesized!  The bold x is missing its protective parentheses.  This was fixed in a Windows 2000 service pack.  For example, here's the Win2K SP3 version:

#define HRESULT_FROM_WIN32(x) ((HRESULT)(x) <= 0 ? ((HRESULT)(x)) : ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)))

Much better.

I'm still personally leery of using side effects in HRESULT_FROM_WIN32 though.  Why?  Because it sets a bad precedent.  It's easier to have a simple rule (no side effects in a macro call!) than to remember for an infinitude of macros and function calls which macros evaluate the arguments multiple times and which function calls really have side effects.  You also get legitimate bugs like this.

Macros, macros, macros... you deviate just a little bit from the straight-and-narrow and what happens?   You get blown out of the water.  Wonderful things, macros.

Nasty little bugs arising from harmless-looking violations of the safety tips are the cause of the dirty secret I am about to reveal...

As of Vista RTM...

HRESULT_FROM_WIN32 is NO LONGER a macro!

This is one of those annoying little facts I try so hard to forget because they complicate life unnecessarily.  From my point of view, there is no bad consequence of treating HRESULT_FROM_WIN32 as the macro it looks like (and was... and who knows, may be again.)  The inline function it has become is uniformly safer.

This has apparently been a long time in coming.  Here are some excerpts of winerror.h from various releases of Windows:

Windows 2000 RTM:

#define HRESULT_FROM_WIN32(x)   (x ? ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)) : 0 )

Windows XP RTM:

// __HRESULT_FROM_WIN32 will always be a macro.
// The goal will be to enable INLINE_HRESULT_FROM_WIN32 all the time,
// but there's too much code to change to do that at this time.

#define __HRESULT_FROM_WIN32(x) ((HRESULT)(x) <= 0 ? ((HRESULT)(x)) : ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)))

#ifdef INLINE_HRESULT_FROM_WIN32
#ifndef _HRESULT_DEFINED
#define _HRESULT_DEFINED
typedef long HRESULT;
#endif
#ifndef __midl
__inline HRESULT HRESULT_FROM_WIN32(long x) { return x < 0 ? (HRESULT)x : (HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000);}
#else
#define HRESULT_FROM_WIN32(x) __HRESULT_FROM_WIN32(x)
#endif
#else
#define HRESULT_FROM_WIN32(x) __HRESULT_FROM_WIN32(x)
#endif

Whew.  The net result is that HRESULT_FROM_WIN32 is a macro.  Oh, unless you decide to be avant-garde and #define INLINE_HRESULT_FROM_WIN32 before you #include <winerror.h>.  Which probably very few people did, except for build engineers running experiments.  So this is an "opt-in" inline function.

Windows Vista RTM:

//
// HRESULT_FROM_WIN32(x) used to be a macro, however we now run it as an inline function
// to prevent double evaluation of 'x'. If you still need the macro, you can use __HRESULT_FROM_WIN32(x)
//
#define __HRESULT_FROM_WIN32(x) ((HRESULT)(x) <= 0 ? ((HRESULT)(x)) : ((HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000)))

#if !defined(_HRESULT_DEFINED) && !defined(__midl)
#define _HRESULT_DEFINED
typedef __success(return >= 0) long HRESULT;
#endif

#ifndef __midl
FORCEINLINE HRESULT HRESULT_FROM_WIN32(unsigned long x) { return (HRESULT)(x) <= 0 ? (HRESULT)(x) : (HRESULT) (((x) & 0x0000FFFF) | (FACILITY_WIN32 << 16) | 0x80000000);}
#else
#define HRESULT_FROM_WIN32(x) __HRESULT_FROM_WIN32(x)
#endif

The experiments appear to have been completed.   Apparently no imaginable consequence of changing to an inline function was worse than the known consequence of bugs.

People who want the macro can still use __HRESULT_FROM_WIN32.

Why would you want to?  Hmmm... maybe you're building a switch statement.  Something like:

#define CASE_HRESULT(x) case x: return #x
#define CASE_ERR(x) case HRESULT_FROM_WIN32(x): return #x // need __HRESULT_FROM_WIN32 here?

LPCSTR FriendlyNameOf(HRESULT hr) {
    switch(hr) {
        CASE_HRESULT(S_OK);
        CASE_HRESULT(S_FALSE);
        ... many more...
        CASE_ERR(ERROR_SUCCESS);
        CASE_ERR(ERROR_INVALID_FUNCTION);
        ...
    }
    return "Unrecognized";
}

I haven't tried it out, but I suspect the compiler will reject the attempt to squeeze the result of an inline function into the value of a case, but will accept a complicated expression involving a bunch of constants (i.e., the macro will work.)

Insight is a tricky thing.  To a certain degree people are born with it (or without it) - but if you are gifted with a measure of it, you can develop it (as though it were a muscle) by working at it.

The late mathematician George Pólya had a number of helpful problem-solving mottos, one of which is the title of this post.  There's a nice echo in the chess world, where the maxim "If you see a good move, wait - look for a better one!" is popular (as is the simpler version, "Sit on your hands!")

The granddaddy of them all is William of Ockham's "entia non sunt multiplicanda praeter necessitatem"... AKA, Occam's [sic] Razor.

KISS, as they say.

What am I going on about?

In the Computer Science world, it happens very often that your first idea... though it works... is inferior to your second idea.  A great tragedy that is played over and over is:

1. Developer is faced with a problem.
2. Developer has brilliant idea.
3. Developer starts coding.  And coding.  For hours.  Straight.
4. After a few hours developer pauses.  Hmm.  Developer has better idea.
5. Developer goes back and hacks up what they wrote to implement the better idea.
6. Goto step 3.
7. Catch out-of-time exception.  Finish the latest iteration as quickly as possible.  Ship it.
   

I'm exaggerating a bit by calling this a great tragedy.  It's also a perfectly good development strategy to begin coding with the knowledge that you will probably come back and redo a large chunk of what you're doing as you become more familiar with the problem domain.  The key words here are "with the knowledge"... if you know that what you're coding is experimental, you can avoid a lot of scheduling Sturm und Drang.

Bottom line: it happens frequently that a good idea has a better one as a sequel.  Keep your eyes open... are you solving a specific case when you could, with less work, solve the general problem?  Look at your fixed code fresh - are you introducing another problem with the fix?  Is there a more appropriate way to fix the problem?  Is it better to fix the symptom instead of the disease?  Stop and think.  Even if you don't find a better way, it's good exercise for your noodle.

There are several ways to get a number that represents "the time right now."  Some of them wrap; some of them don't.

Some that wrap:

• GetTickCount()
• QueryPerformanceCounter()
• timeGetTime()
  

Some that don't:

• time()  (well, this will wrap in 2038... unless you use the 64-bit version... which is the default...)
• DateTime.Now

A common programming meme is to start an operation and then wait for it to complete... but give up after a certain preset "timeout."  There are multi-threaded ways of doing this that are perhaps better, but for better or worse, there is a fair amount of code out there that looks like this:

StartSomething();

while ( ! Done() ) {
    if ( BeenWaitingTooLong(Timeout) ) { break; }

    WaitABit();
}

if ( Done() ) {
    Great();
} else {
    Darn();
}

For this post I want to focus on the BeenWaitingTooLong(Timeout) snippet emphasized above.  If the timing function being used wraps, it is all too easy to get this to work most of the time... when it's just as easy to get it to work all of the time.  Alas, the consequences of software that works most of the time tend to be more severe than software that never works.

I wouldn't last long in the banking business being accurate most of the time! -- The Music Man

I'll use GetTickCount() for these examples, but I want to emphasize that the same malady affects all of the wrappy time counters.

Here are some different ways to do it:

StartSomething();

DWORD msTimeout = 10 * 1000; // give up after ten seconds
DWORD msAtStart = GetTickCount();

// assume Done() always returns within, say, a day
while ( ! Done() ) {
    if (
        // most of the time, these are the same.
        // which one will always work?
        // GetTickCount() - msAtStart > msTimeout
        // GetTickCount() - msTimeout > msAtStart
        // GetTickCount() > msAtStart + msTimeOut
    ) { break; }

    // assume that WaitABit() always returns within, say, a day
    WaitABit();
}

if ( Done() ) {
    Great();
} else {
    Darn();
}

The correct answer is:

GetTickCount() - msAtStart > msTimeout

The other two will work most of the time, but will occasionally fail.

There's an easy rule I use to always remember the right one.

When dealing with a clock that wraps, only compare intervals.

Allow me to redecorate my variable names using Joel Spolsky's "Making Wrong Code Look Wrong" technique.

StartSomething();

DWORD intervalTimeout = 10 * 1000; // give up after ten seconds
DWORD instantStart = GetTickCount();

// assume Done() always returns within, say, a day
while ( ! Done() ) {
    if (
        // which one will always work?
        // GetTickCount() - instantAtStart > intervalTimeout
        // GetTickCount() - intervalTimeout > instantAtStart
        // GetTickCount() > instantAtStart + intervalTimeout

    ) { break; }

    // assume that WaitABit() always returns within, say, a day
    WaitABit();
}

if ( Done() ) {
    Great();
} else {
    Darn();
}

Some thought experiments immediately fall out of this.

• instantA + intervalX = instantB (instantB is later than instantA)
  
• instantA - intervalX = instantB (instantB is earlier than instantA)
  
• instantB - instantA = intervalX  (instantB is later than instantA)
• intervalX - intervalY = intervalZ (intervalZ < intervalX; intervalY < intervalX)
• intervalX + intervalY = intervalZ (intervalZ > intervalX; intervalZ > intervalX)

• instantA + instantB is always a bug due to wrapping
• Yes, even in (instantA + instantB) / 2
• Correct version is instantA + (instantB - instantA) / 2
• instantA < instantB is always a bug