views:

545

answers:

3

A customer recently performed static analysis of my employer's C codebase and gave us the results. Among useful patches was the request to change the famous do { ... } while(0) macro to do { ... } while(0,0). I understand what their patch is doing (using the sequence operator to return evaluate to the value of the second "0", so the effect is the same) but it's not clear why they'd favor the second form over the first form.

Is there a legitimate reason why one should prefer the second form of the macro, or is our customer's static analysis being overly pedantic?

+7  A: 

Well, I'll go for an answer:

Is there a legitimate reason why one should prefer the second form of the macro... ?

No. There is no legitimate reason. Both always evaluate to false, and any decent compiler will probably turn the second one into the first in the assembly anyway. If there was any reason for it to be invalid in some cases, C's been around far long enough for that reason to be discovered by greater gurus than I.

If you like your code making owl-y eyes at you, use while(0,0). Otherwise, use what the rest of the C programming world uses and tell your customer's static analysis tool to shove it.

Chris Lutz
It does not evaluate to anything; and do-loop is not an expression and has no value. Try assigning it int x = macro() ;, and it fails to compile.
Clifford
I'm referring to the evaluation of `0` vs. `0,0`, not the `do { } while(0)` loop in general.
Chris Lutz
Ok; I was still trying to fathom the benefit as a whole.
Clifford
What benefit​​?
Chris Lutz
The continuation expressions are not what they seem.
Steve Jessop
Thanks. I couldn't think of a good reason and I'm glad the Internet confirmed it for me.
Commodore Jaeger
"What benefit?": The benefit conveyed by using 0,0 in the while loop condition. The static analysis seems to believe there is one, but this answer, though accepted does not reveal that so we are left wondering.
Clifford
My point (and the point of my answer) is that there _is_ no benefit. Any benefit the static analysis tool imagines to be there cannot possibly exist.
Chris Lutz
+1 for being sane about macros
epatel
@Chris: I get *your* point, but out of curiosity, I was hoping that someone could enlighten us as to why the tool was advising this - even if the thinking were flawed. I can find no reference to such an idiom anywhere. Even teh MISRA C guidelines do not advise this. It would help if the OP were to tell us what the tool was perhaps?
Clifford
It would be nice to know, but it's a tool that the OP says was used by a customer, so we may never get to find out.
Chris Lutz
+1  A: 

I understand what their patch is doing (using the sequence operator to return the value of the second "0" [...]

Return it to what? I don't understand why that is necessary at all. Any good static analysis tool will explain it's directives - what does it say?

Personally I'd be more concerned about the use of overly complex macros in the first instance rather whatever problem this is intended to solve.

Clifford
+1, but I'm kinda iffy on the whole "macros are evil" vibe. They're a tool, and like any tool can be abused, but they aren't evil. I happen to think `#define STRCMP(s1, op, s2) (strcmp(s1, s2) op 0)` is rather elegant in its use: `STRCMP("Hello", ==, "Hello")` Hardly ideal, but quite clever, and certainly works.
Chris Lutz
@Chris: I hope that your tongue was firmly in you cheek when you wrote that!? ;-) A perfect example of a gratuitously pointless macro. However the use of a do-while implies a macro with multiple statements, which is often a bad idea; from a debugging point of view if nothing else.
Clifford
Perhaps elegant was too strong a word. It's not elegant, but it certainly conveys a meaning (is one string equal to another) with more clarity than `!strcmp(s1, s2)` (if we want equality, we shouldn't have to negate something!). It's a macro I wouldn't ever use, but I think it's a clever way to subvert the preprocessor. More importantly, it's an issue of choosing between "this code says what I'm doing" and "this code says what I want to accomplish." I believe that, if we want to test whether two strings are equal, we should be using `==` somewhere. Anyway, I wouldn't use it in any real code...
Chris Lutz
...because people would hang me for it. But I wouldn't hang anyone else who used it. It's seeking conceptual clarity at the expense of syntactic clarity. I'd prefer conceptual clarity, but I can understand (but am not totally swayed by) the arguments against it in cases like this. I certainly wouldn't penalize someone for using a multi-statement macro in a `do { ... } while(0)` loop. It's hackery, but it's so common (and it's uses/limitations are so widely known) that discouraging it when it might be appropriate is silly.
Chris Lutz
`strcmp(s1, s2)` is not a boolean expression so I would not have written `!strcmp(s1, s2)` in the first instance, but rather `strcmp(s1, s2) == 0` - which I suggest is clear enough. If you want true clarity, then perhaps the need for this kind of macro is actually telling you that it is time to move to C++ and std::string.
Clifford
+6  A: 

Just a guess as to why they might suggest using

do { ... } while(0,0)

over

do { ... } while(0)

Even though there's no behavior difference and should be no runtime cost difference between the two.

My guess is that the static analysis tool complains about the while loop being controlled by a constant in the simpler case and doesn't when 0,0 is used. The customer's suggestion is probably just so they don't get a bunch of false positives from the tool.

For example I occasionally come across situations where I want to have a conditional statement controlled by a constant, but the compiler will complain with a warning about a conditional expression evaluating to a constant. Then I have to jump through some hoops to get the compiler to stop complaining (since I don't like to have spurious warnings).

Your customer's suggestion is one of the hoops I've used to quiet that warning, though in my case it wasn't controlling a while loop, it was to deal with an "always fails" assertion. Occasionally, I'll have an area of code that should never execute (maybe the default case of a switch). In that situation I might have an assertion that always fails with some message:

assert( !"We should have never gotten here, dammit...");

But, at least one compiler I use issues a warning about the expression always evaluating to false. However, if I change it to:

assert( ("We should have never gotten here, dammit...", 0));

The warning goes away, and everybody's happy. I'm guessing that even your customer's static analysis tool would be, too. Note that I generally hide that bit of hoop jumping behind a macro like:

#define ASSERT_FAIL( x) assert( ((x), 0))

It might be nice to be able to tell the tool vendor to fix the problem, but there might be legitimate cases where they actually do want to diagnose a loop being controlled by a constant boolean expression. Not to mention the fact that even if you convince a tool vendor to make such a change, that doesn't help you for the next year or so that it might take to actually get a fix.

Michael Burr
+1 This might be it, but if this is the case these warnings should just be disabled with command line options. Why jump through hoops when you can just turn off the hoops that aren't necessary?
Chris Lutz
Well, for one, futzing around with compiler or tool options might be considered a form of hoop jumping. Screwing around with the makefiles or whatever controls your build is often much more black magic than C preprocessor hackery is (and that's saying a lot). For another, you might not want the warning to be disabled for everything - maybe just for the debugging macros that seem to need these stupid tricks.
Michael Burr