When is a WHILE Not a while?

I have a pretty good life.  There are a lot of indicators of this, but one in particular is that I have the luxury of being annoyed by things like the issue that’s the topic of this blog post.

I was reading some code from Microsoft the other day, and I noticed that a loop I was reading was contained in a WHILE block.  Note that’s not the same as a while block (lower case)… the code was something like this:

do
{
 
    if(!FdoData->AllowWakeArming) {
        status =  STATUS_NOT_SUPPORTED;
        break;
    }
 
    pPmPattern = (PNDIS_PM_PACKET_PATTERN) InformationBuffer;
 
    if (InformationBufferLength < sizeof(NDIS_PM_PACKET_PATTERN))
    {
        status = STATUS_BUFFER_TOO_SMALL;
 
        *BytesNeeded = sizeof(NDIS_PM_PACKET_PATTERN);
        break;
    }
 
//
// ... tons of stuff deleted to avoid boredom ...
 
    status = STATUS_SUCCESS;
 
} WHILE (FALSE);

Hmmmm… What’s this?  A WHILE macro? I searched the header file and sure enough, there it was:

//-----------------------------------------------------------------------------
// 4127 -- Conditional Expression is Constant warning
//-----------------------------------------------------------------------------
#define WHILE(constant) \
__pragma(warning(suppress: 4127)) while(constant)

Yes, folks… there’s a WHILE macro.  And its entire purpose is to disable the (warning level 4) compiler warning “Conditional Express is a Constant.”  I mentioned this to Scott.  His response: “That’s all over the 8.1 WDK samples, so we’ll be stuck with WHILE for, er, a while.”

Want to know what’s worse?  There’s more than one version of the WHILE macro.  It’s a per-project thing.  Here’s an uglier and sillier version:

// 4127 -- Conditional Expression is Constant warning
#define WHILE(constant) \
__pragma(warning(disable: 4127)) while(constant); __pragma(warning(default: 4127))

Hey, at least all the variations I’ve found so far accomplish the same goal.  I’m thankful for that at least.

But seriously: I find this type of programming construct annoying in the extreme.  It’s the kind of “hey, look at this… isn’t this clever” kind of trick that I frequently see in the work of relatively junior devs.  You know, the kind who’d think (if not actually say) “Hey, while you were doing the review of my 7,500 lines of code, I bet you didn’t notice that one of those while statements actually was in upper case!  Wow, you’re not nearly as good as you think you are, and I am super-duper clever!  You should read more carefully.”

Now, let me be clear: I don’t hate macros in general (or, at least, not very much).  I don’t take umbrage over the concept of suppressing C4127.  And I certainly applaud building code that is /W4 clean.

What annoys me so dreadfully is burying this suppression in a macro that differs only by case with the actual C Language keyword, and then mixing the use of both the macro and the (lower case) C Language keyword in the same program.  This is precisely the same kind of ire I reserve for overloading arithmetic operators for standard variable types in C++.

While I sympathize with a general dislike for the ugliness, leaving the pragma to suppress the warning in-line and visible within the code clearly warns the reader that the original developer intended the suppression:

    do
    {
 
        if(!FdoData->AllowWakeArming) {
            status =  STATUS_NOT_SUPPORTED;
            break;
        }
    
    //
    // ... Even more stuff deleted ...
    // 
        status = STATUS_SUCCESS;
 
__pragma(warning(suppress: 4127))
    } while (FALSE);

Perhaps not gorgeous, but heck… it’s freakin’ C, people.  It’s never gorgeous.

Or, let’s say you *really* hate seeing the pragma.  Or you can’t remember the pragma syntax or the warning number that you want to suppress and you’re deeply in love with the idea of  using constants in your while statement (the merits of which are another topic altogether that we could debate for days).  If that’s the case, at least make what you’re doing obvious.  Call attention to it.  Please.  Do something like this:

in the header:

#define WHILE_FALSE \
__pragma(warning(suppress: 4127)) while(FALSE)

then, do something like this in the code:

do
{
 
    if(!FdoData->AllowWakeArming) {
        status =  STATUS_NOT_SUPPORTED;
        break;
    }
 
//
// ... useful work here ...
// 
    status = STATUS_SUCCESS;
 
 
} WHILE_FALSE;

I don’t like this very much either, but I like it a whole lot better than a macro that differs only by case for this purpose.

One of the primary goals in writing source code is creating code that’s clear, simple to understand, and maintainable.  I agree that an important part of those goals entails making your code easy to read.  And I agree that junking your source code with a bunch of pragamas and conditionals (and SAL annotations)  can certainly make your code harder to read.

But “easy to read” means not only “scannable” and clean, but also “understandable.”   When you hide what you’re doing with an easy to miss macro, you’re actually working against the goal of making your code understandable.

The WDK Samples should showcase best-in-class coding practices, not just the way to call a given set of DDIs.

Now I feel better. Whew!