Tag Archives: quiz

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

Check out my latest book, the essential, in-depth guide to performance for all .NET developers:

Writing High-Performance.NET Code, 2nd Edition by Ben Watson. Available for pre-order:

What’s Wrong with this Code 2

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!


Check out my latest book, the essential, in-depth guide to performance for all .NET developers:

Writing High-Performance.NET Code, 2nd Edition by Ben Watson. Available for pre-order:

What’s Wrong with this code? – 2 – Answer

In the last post, I showed some code that had a very simple problem.

The problem is that when you call HIWORD, it converts the 16 bits into an unsigned short, which then gets passed as an int padded with zeros–not sign extended (it’s not signed). It will NEVER be less than zero. The solution is to cast it to a signed short before calling the function or change the function parameters to be signed short instead of int.

 


Check out my latest book, the essential, in-depth guide to performance for all .NET developers:

Writing High-Performance.NET Code, 2nd Edition by Ben Watson. Available for pre-order:

What’s Wrong with this code? – 2

What’s wrong with this code?

UINT a;
.
.
.
DoSomething(HIWORD(a));
void DoSomething(int x)
{
 if (x < 0)
 {
  //do something
 }
 
}

I recently ran into this in some code I was working on.


Check out my latest book, the essential, in-depth guide to performance for all .NET developers:

Writing High-Performance.NET Code, 2nd Edition by Ben Watson. Available for pre-order:

What’s wrong with this code? – 1 – Answer

The main thing wrong with the code is that object.ReferenceEquals(a,b) will ALWAYS return true because it’s comapring structs, which are value types. Value types are passed by value, not reference, so every instance is different.

 


Check out my latest book, the essential, in-depth guide to performance for all .NET developers:

Writing High-Performance.NET Code, 2nd Edition by Ben Watson. Available for pre-order:

What’s wrong with this code? – 1

What is wrong with the following code? 

struct Foo {
    public int id;
    public string value;
    public static readonly Foo Empty = new Foo(“”);

    public Foo(string val)
    {
         this.value = val;
         this.id = -1;
     }

};

and elsewhere…

Foo m = new Foo(“Something”); 

if (object.ReferenceEquals(m, Foo.Empty))
{
    …//do something
} else
{

}

 


Check out my latest book, the essential, in-depth guide to performance for all .NET developers:

Writing High-Performance.NET Code, 2nd Edition by Ben Watson. Available for pre-order: