In my previous post, I showed some code that we “fixed” and it caused problems–it actually broke the algorithm.

The bug was in relying on some compiler behavior and the usage of the stack.

Here’s the original (pre-“fix”) code again for reference:

                 for(i = 0; i < overlaySize.cx; i++) {
                    long i1 = (long)((double)i / numXTimes);
                    double xPercent = (double)i / numXTimes - i1;

                    // get the indeces into the data array
                    long lIndex1 = i1 + (j1 * m_DataSize.cx);
                    long lIndex2 = lIndex1 + m_DataSize.cx;

                    double yVal1, yVal2;
                    if(i1 != oldi1) {
                        yVal1 = m_DataArray[lIndex1] + //etc...

                        yVal2 = m_DataArray[lIndex1 + 1] + //etc...

                        oldi1 = i1;
                    }

                    // figure out the value
                    double theVal = yVal1 + (yVal2 - yVal1) * xPercent;
                    

                }

The answer lies in the realization that yVal1 and yVal2 need to retain their values through each iteration of the loop. It always worked before because the compiler ensured that each time those variables are used, even if they’re not initialized after declaration, they reference the same place in the stack on each loop iteration. By setting them to 0.0 each time, we’re breaking the reuse functionality. Finding this, we realized that is horrible! At the very least, it’s brittle, prone to break if anything went into the loop which altered the stack differently on different iterations.

Thankfully, it was easy to fix. Just move the declarations and initialize them outside the loop.

                double yVal1 = 0.0, yVal2 = 0.0;

                for(i = 0; i < overlaySize.cx; i++) {
                    long i1 = (long)((double)i / numXTimes);
                    double xPercent = (double)i / numXTimes - i1;

                    // get the indeces into the data array
                    long lIndex1 = i1 + (j1 * m_DataSize.cx);
                    long lIndex2 = lIndex1 + m_DataSize.cx;


                    if(i1 != oldi1) {
                        yVal1 = m_DataArray[lIndex1] + //etc...

                        yVal2 = m_DataArray[lIndex1 + 1] + //etc...

                        oldi1 = i1;
                    }

                    // figure out the value
                    double theVal = yVal1 + (yVal2 - yVal1) * xPercent;
                                

                  }

I don’t know who wrote the brain-dead code in the first place, but…wow.

Anyway, lessons learned?

1. Test a change like this–I had put some asserts there because I didn’t fully understand what was going on, but it was so long until I ran the code after making the change that I had mostly forgotten about the change.
2. Turn on strict compiler settings from the get-go. Make it force you to be ultra-precise. It can be VERY painful changing 120,000 lines of C++ code to be warning-free at L4. (But we did it!)
3. Implement and change separate things separately. While removing compiler warnings, we also converted the codebase to Unicode–most places were using _T(“”), but a few places such as file handling needed special attention. That distracted from finding this problem sooner (“Could it be a file reading problem?”)

We’ve been working on our next version of our flagship product at work and, as part of that, we upgraded to Visual C++ 8 (2005) and turned on the most strict compiler settings, warning level 4, more debug runtime checks–basically trying to make everything very strict.

As part of that process, we had to go through the code and fix hundreds (thousands?) of warnings that just became errors.

One area was the following:

                 for(i = 0; i < overlaySize.cx; i++) {
                    long i1 = (long)((double)i / numXTimes);
                    double xPercent = (double)i / numXTimes - i1;

                    // get the indeces into the data array
                    long lIndex1 = i1 + (j1 * m_DataSize.cx);
                    long lIndex2 = lIndex1 + m_DataSize.cx;

                    double yVal1, yVal2;
                    if(i1 != oldi1) {
                        yVal1 = m_DataArray[lIndex1] + //etc...

                        yVal2 = m_DataArray[lIndex1 + 1] + //etc...

                        oldi1 = i1;
                    }

                    // figure out the value
                    double theVal = yVal1 + (yVal2 - yVal1) * xPercent;
                }

I’ve of course cut out a number of lines from this loop that weren’t relevant to the point.

The compiler flagged yVal1 and yVal2 as being potentially unitialized before they were used. It’s because they’re initialized inside an if statement.

So to remedy this, I initialized them to 0.0:

double yVal1=0.0, yVal2=0.0;
if(i1 != oldi1) {

//etc.

I go on to fix other warnings-become-errors, and we finally create our first build with VC8 a week or two later. Then, we start having problems in one area of the program. A data file is working on looks horrible, and it’s taking 45 minutes to do the calculations instead of 2-3 seconds. What’s going on? It took a while to find it, but find it I did…

There is a serious bug in the code above, which my initializing the variables hid. The answer next time!