What’s wrong with this code?

This one comes courtesy of a co-worker who once upon a time was the lead developer for the Manx (http://www.itee.uq.edu.au/~csmweb/decompilation/hist-c-pc.html) compiler.

He ran into the following problem that was reported by the customer.  They tried and tried to get the following code to compile and it didn’t seem to work:


#include <stdio.h>
float ReadFileAndCalculateAverage(FILE *InputFile, int Values[], int LengthOfValues, int *NumberOfValuesRead)
   float average;
   int i;

   // Read the Values array from InputFile.

   <Code eliminated for brevity>

   // Now calculate the average.

   for (i = 1 ; i <= *NumberOfValuesRead ; i += 1)
      average += Values[i];

   average = average/*NumberOfValuesRead;

int main(int argc, char *argv[])
    int Values[20];
    float average;
    int numberOfValues;
    FILE *inputFile;

    inputFile = fopen(“MyFile.Txt”);
    if (inputFile != NULL)
        average = ReadFileAndCalculateAverage(inputFile, Values, sizeof(j) / sizeof(j[0]), &numberOfValues);

 Now imagine this problem in a 10,000 line source file πŸ™‚ Have fun trying to track it down.

Edit: Removed bogus <p>’s.  Thank you Newsgator’s .Text plugin…

Comments (27)

  1. Anonymous says:

    /* is a comment, not division followed by pointer dereference πŸ™‚

  2. Anonymous says:

    there is no j.. what is j?

    and /* is not what you think it is there

  3. Anonymous says:

    Grr – left the ‘j’ from a previous version. πŸ™‚

  4. Anonymous says:

    I thought any sane developer uses an editor/IDE with syntax highlighting. Any software capable of this, be it Visual Studio, VIM or even EMACS would show that code as if it was a comment. Pretty trivial if you ask me

  5. Anonymous says:

    To my knowledge, vi doesn’t, neither do many versions of emacs (some do, some don’t).

    Other editors optionally enable syntax coloring.

  6. Anonymous says:

    you’re probably thinking of VIM, which, when installed, creates a symbolic link vi that actually starts vim with syntax highlighting

  7. Anonymous says:

    float average in ReadFileAndCalculateAverage is uninitialized

  8. Anonymous says:

    In main, j is undeclared. It seems that j should be Values. Ignoring the eliminated code, it seems that average may be uninitialized. Maybe that was eliminated for brevity. The for loop starts at 1. It should start at 0 and use < rather than <=. After the loop it might be sensible to test for there being 0 values read…

    /* is already pointed out… That is the tricky part, but the other errors are pretty serious.


  9. Anonymous says:

    the lack of a return statement

    (in addition to the /*)

  10. Anonymous says:

    also fopen requires a 2nd parameter specifying the access permitted to "MyFile.Txt"

  11. Anonymous says:

    Vim does syntax highlighting. Only a masochist would use vi.

  12. Anonymous says:

    Hey, whaddya know, open source works! πŸ™‚

  13. Anonymous says:

    fclose should be inside the if block. I’m not sure what the effect of trying to fclose a NULL pointer is, but I’m not going to take the risk.

  14. Anonymous says:

    What about the array? You reserve space for a value[20] array but you don’t limit the number of values you read. So when you compute the average you might try to access value[x] where x > 20?

  15. Anonymous says:

    Easy. The filename parameter to fopen() uses typographical quotation marks instead of straight quotes.

    That is:

    inputFile = fopen(β€œMyFile.Txt”);

    should be:

    inputFile = fopen("MyFile.Txt");

    This is what you get when newspaper editors write code πŸ˜‰

  16. Anonymous says:

    I pasted it into emacs to compile and see what kind of error it would get, but the syntax highlighting made it instantly obvious what was wrong.

  17. Anonymous says:

    So what’s the count now?

    1. /* comment and pointer dereference


    2. fopen call missing an argument

    3. fopen call with bad string constant

    4. Loop to calculate average has bad bounds

    5. ReadFileAndCalculateAverage does not return a value

    6. sizeof(j) should be sizeof(Values)

    7. fclose outside of if(inputFile != NULL)

    8. NumberOfValues (presumably set in <Code eliminated for brevity>) read might be larger than 20 — probably just testing code

    Is that all? g++ says yes.

  18. Anonymous says:

    That’s probably it. Of course I only intended the /* one to be relevant but that’ll teach me to write code on the fly without even trying to compile it.

    In the original case, this came from a 10,000 line source file, which reported an error at about line 9,000. The offending /* was at line 1,000 or so in the file. But of course the compiler error pointed to line 9,000 as being at fault, and the customer couldn’t see why on earth that was the case.

    We won’t discuss the fact that he had a source file with 8,000 lines without a single comment in them.

    Oh, an as for syntax highliters. Many turn off their syntax coloring logic after the first line of an unclosed comment. This also occurred long before editors used syntax coloring – back in 1986 when this occurred, no text editor worth it’s salt would do syntax coloring, it was too CPU intensive.

  19. Anonymous says:

    ‘i’ is initialised to 1, but should this be zero?

    for (i = 1 ; i <= *NumberOfValuesRead ; i += 1)

    should be:

    for (i = 0 ; i < *NumberOfValuesRead ; i++)

    Who uses "i += 1" πŸ™‚

  20. Anonymous says:

    I use "i += 1" almost exclusively. For legacy reasons, I utterly dispise i++.

    The only reason to use i++ is if you’ve got a compiler like the generic c compiler from back in the 1980’s that required the ++ construct to enable it to take advantage of the processor’s INC instruction.

    But modern compilers shouldn’t have this difficulty.

  21. Anonymous says:

    Oh, and you’re right Paul, it should be 0, it’s a legacy of the original code I wrote up for this post.

  22. Anonymous says:


    you really continue to entertain me /w code.

    please keep up these posts. great stuff.


    thomas woelfer

  23. Anonymous says:

    Keep up the good work , Larry! I want more!

  24. Anonymous says:

    ++i is faster than i++ or i+=1

  25. Anonymous says:

    yarbo, really? I’d be really quite surprised if "++i", "i++" or "i+=1" generated different code.

    Especially when you consider that:




    i += 1;

    are semantically identical.

    If the i++ or the ++i is used as a term in an expression, then their semantics are different, and it’s possible that the code generation for ++i might be slightly better than i++ or i+=1, but as a stand alone statement, the compiler should be treating them the same.

  26. Anonymous says:

    Yes, as a statement and with integers preincrement ++i, postincrement i++ and increment-by-one i+=1 are equivalent. They probably generate the same code.

    But if i were an object with a custom operator++() and operator++(int), the postincrement is usually implemented as

    const T T::operator++(int)


    T old(*this); // remember our original value

    ++*this; // always implement postincrement

    // in terms of preincrement

    return old; // return our original value


    and, in order to optimize the postincrement into a preincrement (dropping the copy construction), the compiler would have to (0) know that postincrement and preincrement on Class have the same semantics except for the return value, (1) detect that the return value of operator++(int) is discarded (which may be fairly easy), and (2) know that the Class copy constructor and destructor have no side effects (which may not be so easy).

    So, as prophet Sutter teaches us[1], we should develop a habit of avoiding postincrement unless we really need the old value. Even then, I usually split the statement in two, with ++i being the second half.

    That said, of course, i += 1 is the same as ++i for ints. Again, for objects, it may or may not be true. For example, when dealing with iterators, i += 1 requires a random access iterator, while ++i can be used with any category of iterators.

    [1] Herb Sutter. Exceptional C++: 47 Engineering Puzzles, Programming Problems, and Solutions. – Addison Wesley, 1999. – ISBN 0-201-61562-2. – Item 6. Temporary Objects

  27. Anonymous says:

    Fair ‘nuf. But then again, I avoid operator overloads like the plague, as should every systems engineer (see http://weblogs.asp.net/larryosterman/archive/2004/04/06/108686.aspx, and the comment by Michael Grier here:http://weblogs.asp.net/larryosterman/archive/2004/04/06/108686.aspx#113633).

    One day I’ll write "Larry’s rules of software engineering #<n>: Almost everything you find in C++ should be avoided like the plague." πŸ™‚ I haven’t because much of it is simply a restatement of rule #1, but.

    So when I write things like i++, I’m almost always dealing with POD – heck, I’m not sure I’ve ever used arithmetic operator overloads (I’ve overloaded operator == a couple of times but that’s about it).

    And since I know the assembly language generated by my code (roughly) I know the consequences.