Peeter Joot's (OLD) Blog.

Math, physics, perl, and programming obscurity.

scope conflict between member variable and loop initializer.

Posted by peeterjoot on October 19, 2009

Here’s a scoping gotcha that bit me today … scratched my head over this one for a bit in the debugger before I figured it out.

I was working on some code that had some uninitialized variables, one of which was like so:

unsigned i ;
// pages of junk
for ( i = 0 ; i < blah ; i++ )
{
}
...
for ( i = 0, m_confused = 0 ; i < totalIterations ; i++ ) // m_confused is a member variable.
{
}

There were some other uninitialized variables, and I gave these all explicit invalid or zero values to make sure they didn’t have stack garbage in them. For the loop counter i, I just moved it into the for () declaration like so:

for ( unsigned i = 0 ; i < blah ; i++ )
{
}
...
for ( unsigned i = 0, m_confused = 0 ; i < totalIterations ; i++ )
{
   if ( stuff )
   {
      m_confused = N ;
      break ;
   }
}

Did you spot my mistake? I didn’t when I made the change, and it was so minor that I forgot I’d even made it by the time I tried out the code.

After the loop termination my member variable m_confused was mysteriously set to zero, despite the loop termination condition in the non-error codepath requiring that a value be assigned.

Turns out that the compiler (g++ 4.2.3 in this case) after I “fixed” the code to remove the possibility of referencing the loop counter inappropriately interpreted this as me declaring a loop scoped variable m_confused. That, and not this->m_confused got my update, so once it went out of scope I was left with zero in my class. Unfriendly of the compiler to not give me a warning about this even with -Wall (the older gcc 4.1.2 compiler does).

What are the lessons to be learned?

  • don’t fix unbroken code.  This unfortunately takes much more self discipline than I have!
  • no more than one statement per line.  Keeping the code simple, should give less chance for you to misinform the compiler what you want to do.  There wasn’t any good reason that the original code needed to initialize both of these in the for loop.  The ‘m_confused = 0’ statement could have just as easily been on it’s own line.  Ironically, this particular value was already zeroed out in other code, so it didn’t even have to be in the for loop explicitly at all.
  • always initialize all stack variables no matter what at the point of declaration.  Even the simple change to make it obvious that it is initialized by restricting the usage scope can be screwed up.
  • have good code coverage testing.  If I had been unlucky enough to have made this “maintainability-fix” in a non-mainline codepath, I’d have made a very unfriendly alteration for somebody else (or myself) to later debug.
Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

 
%d bloggers like this: