What’s Wrong with this Code 2 (Answer)

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?”)