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:

Main.c:

#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);
    }
    fclose(inputFile);
 }

 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. Jerry Pisk says:

    /* is a comment, not division followed by pointer dereference :)

  2. David Dollar says:

    there is no j.. what is j?

    and /* is not what you think it is there

  3. Grr – left the ‘j’ from a previous version. :)

  4. Alex 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. To my knowledge, vi doesn’t, neither do many versions of emacs (some do, some don’t).

    Other editors optionally enable syntax coloring.

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

  7. me says:

    float average in ReadFileAndCalculateAverage is uninitialized

  8. Ray Seyfarth 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.

    Ray

  9. Ted Warner says:

    the lack of a return statement

    (in addition to the /*)

  10. Chris Bopp says:

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

  11. ed says:

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

  12. Dave says:

    Hey, whaddya know, open source works! :)

  13. Mike Dimmick 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. Dominik 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. 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. chris green 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. So what’s the count now?

    1. /* comment and pointer dereference

    confusion

    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. 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. Paul 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. 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. 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. Larry,

    you really continue to entertain me /w code.

    please keep up these posts. great stuff.

    WM_THX

    thomas woelfer

  23. Human Prograamming God says:

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

  24. yarbo says:

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

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

    Especially when you consider that:

    i++;

    ++i;

    and

    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. Centaur 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. 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.