How not to fix a build warning

The Hollywood writers strike continues, and the desperation grows for alternative sources of entertainment. Fortunately, we programmers can find entertainment in our own sources. I’ve got some reality programming for you! The following snippet of code is taken from an actual CVS commit. (Yes, CVS. Don’t laugh. Do cry for me, Argentina.) This build warning ‘fix’ was made by some contractor for some project that I worked on at some point in time for some company. To protect the innocent and/or guilty, I won’t say who, what, when, or where. As for why, I wish I knew. Or maybe not.

NSEnumerator* fileEnum = [fileArray objectEnumerator];
NSDictionary* aDict = nil;
//Changed to Remove the Build Warnings
//while(aDict = [fileEnum nextObject])
while(aDict == [fileEnum nextObject])

Let this example serve as a lesson. Not for programmers — the one who wrote it is probably hopeless — but rather for managers. Please do not just hire the lowest bidder!

17 Responses to “How not to fix a build warning”

  1. Hah! Oh, what a splendid example. Took me a minute to spot it, even when looking for a flaw. So great, the powers of == vs = are.

  2. Lee says:

    I never liked assignment inside if() and while(). This is just another good example of why devs should avoid it.

  3. Michael Campbell says:

    The compiler WARNED about it. It’s not something the devs should avoid, it’s something they should heed the compiler’s warnings about.

  4. Mio says:

    What is this written in? I think C# has a preprocessor directive to supress compiler warnings like that if you want to.

  5. Chris Connett says:

    Except here you *don’t* want to heed the compiler’s warning, if I intuit the code’s intended behavior correctly. This developer blindly changed the code because the compiler warned that assignment in a while condition *might* be an error. They didn’t understand the code enough to know that the compiler was wrong.

  6. she says:

    looks like obj-c

  7. Brian says:

    For those unfamiliar with Objective-C, the “correct” fix would be:

    while((aDict = [fileEnum nextObject]))

    The warning emitted by gcc is something along the lines of “suggest double parentheses around assignment used in truth value”.

    Of course, whether adding an extra set of parentheses to disambiguate this case is enough or the correct solution is open to debate. But changing the semantics of the program certainly isn’t correct.

  8. John says:

    This is also a great advertisement for commenting. For any syntactically tricky code, it should say what it is doing and why. Hoping every subsequent developer will understand is not the way to go.

  9. Chuck says:

    How on earth did that get committed? I mean, it’s not just stupid code — it can’t possibly have worked in any situation. It would fail to do anything if there were files in the array and it would go into an infinite loop when there weren’t.

  10. Steve says:

    Thank god he left the previous line of code there for comparison.

  11. me says:

    Thanks Brian for pointing that out, because the previous posts were making for equally infuriating and funny reading.

    The similarity between = and == ? How about the similarity between Duck and Suck? We’ve got a loose canon there guys. Watch out or pretty soon we’ll have childrens’ shows where the presenters utter obscenities by accident.

    Not using assignment in while? Oh, lord forbid using a clearly defined feature in the language.

    For those who have problems with these operations: wake up and read the reference just once. If the reference is too much for you, just read a tutorial which will mention these in the opening chapters. Not every language is Visual Basic.

  12. Saurabh says:

    The correct fix would be to add/drop a compiler flag so this warning doesnt show. If there’s no such flag, curse the compiler, then write a script to filter out compiler “noise” so you can get at the real warnings, but *leave the code be*. It is perfectly valid, standard code, and there should be no need for (by-definition non-standard) roundabouts like — @Brian — double parens etc to “satisfy” the compiler. What if the same code has to be compiled on another compiler which wants say (x=y)!=0 as the “correct” fix?

    One thing I’d recommend is commenting assignments inside if/while etc. Even something as trivial as “// Yes, = is intended” should be sufficient. I’ve had something similar happen to me once (someone added a break to a switch-case I’d written correctly without a break, they thought it was left out by mistake). Dont underestimate the adage “a little knowledge is a dangerous thing”.

  13. Jeff says:

    For reference, the warning is generated by the compiler option -Wparentheses. That option is enabled by -Wall and disabled by -Wmost.

  14. Pierre says:

    The original line should never have been written thus.

    The correct way to do it is:

    while((aDict = [fileEnum nextObject]) != NULL)

    (or whatever ‘null’ passes for in that language).

    The programmer should not indulge in clever terseness at the expense of clarity and maintainability.

  15. Haha, “nice” solution :)
    Good thing the guy left the comment…

    What is this written in? I think C# has a preprocessor directive to supress compiler warnings like that if you want to.

    GCC has that too. But we, ObjC programmers, turn on ALL compiler warnings (part of that is due to the dynamic nature of ObjC).

    Brian pointed out the correct way to avoid this warning.

    P.S. One warning I hate is when you don’t add a new line after if () without braces – gcc tells us that we possibly missed the braces.

  16. Lee: Actually, this example is used widely in Objective C 1.x code for enumerators. Though, in ObjC 2 it’s being replace by foreach.

  17. jonathan says:

    Thats very funny. type equality, instead of assignment & . I agree with some of comments, about this being unclear, however objective-c programmers and the related examples are commonly done in this way.

    The contractor isn’t necessarily a fool, however this was dumb, that much is clear. However i believe the author should have commented this. Especially if it caused a build warning, but was actually correctly implemented.