#defines are EVIL (part II)

In a previous post I discussed some aspects of the C preprocessor (hereafter the CPP) that are evil. Turns out that this week, I had another problem related to a bad usage of the CPP. It didn’t take long to fix, but I can understand why it could be long to figure out.

And while the bug was caused by a careless use of the CPP, I think there’s a couple of simple things we can do to help avoid these.

The problem I had revolved around the fact that a simple line of code such as:

#ifdef CLUSTER_PARAM_DEBUG
 ...
#endif

can’t be validated automatically in any way. It merely tests for CLUSTER_PARAM_DEBUG to be defined, and if not, nothing special happens. It cannot check if this is the intended spelling, nor if the use of this particular symbol is consistent with the coders’ intention. If it’s defined, the code (represented by the ellipsis in the snippet) gets compiled, otherwise, it is removed from the source code just prior to compilation.

The fact that symbols can be defined or not pose a problem not found in many other programming languages (OK, technically, the CPP is more meta-programming than programming, but just follow for now) as in many other programming languages, symbols, whether functions or variables, need to be declared and assigned a value. If you were to use a symbol in C, as in:

if (cluster_param_debug)
 {
  ...
 }
else
 {
  ...
 }

The compiler would complain that cluster_param_debug is undefined, and cause an error, rather than assuming it has an undefined value (0 would be the most sensible for ‘undefined’ in C and C++) and compile the code anyway.

The fact that the CPP doesn’t require the symbols to be defined always (and assigned true/false or other values) unleashes a plethora of not that easy to spot problems. Consider this made-up code snippet:

double last_error=std::numeric_limits<double>::max();
double this_error=std::numeric_limits<double>::max();

#ifdef CLUSTER_PARAM_DEBUG_MODE
 std::cout << "start nb_colors=" << nb_colors << std::endl;
#endif
do
 {
  last_error = this_error;

  // we just find which color
  // goes with which centroid
  //
  std::vector<rgb_counts> new_centroids(nb_colors);
  std::vector<int> best_centroids(unique_colors.size());

#ifdef CLUSTER_PARAM_DEBUG
 std::cout << "iter=" << iter << " nb_colors=" << nb_colors;
#endif

  for ( size_t this_color=0;
        this_color<unique_colors.size();
        this_color++)
   {
    int i=closest(unique_colors[this_color],centroids);
    best_centroids[this_color]=i;
    new_centroids[i]+=unique_colors[this_color];
   }

  // new centroids! (automagic
  // casting takes care of everything)
  //
  for (int i=0;i<nb_colors;i++)
   centroids[i]=new_centroids[i];

  // compute error with
  // new centroids
  //
  this_error=0;
  for ( size_t this_color=0;
        this_color<unique_colors.size();
        this_color++)
   this_error+=unique_colors[this_color]-centroids[closest(unique_colors[this_color],centroids)];

 # ifdef CLUSTER_PARAM_DEDUG_MODE
  std::cout << " error=" << this_error << std::endl;
#endif
 } while (last_error/this_error > 1.0005 );

Can you spot the (at least) two problems with it (neglecting the actual C++ contents)?

First, there are two symbols, CLUSTER_PARAM_DEBUG_MODE and CLUSTER_PARAM_DEBUG that are conspicuously linked; the second one being a mistype of the first. With very high probability, the coder meant to use CLUSTER_PARAM_DEBUG_MODE but somehow typed CLUSTER_PARAM_DEBUG, and didn’t check much. The second error takes careful reading to spot. The second CLUSTER_PARAM_DEBUG_MODE is in fact spelt CLUSTER_PARAM_DEDUG_MODE, which is only a typo, but still yield unwanted (and undetected at compile-time) effects.

*
* *

What can we do as a sanity check to prevent this? With the CPP alone, there’s simply nothing we can do. The fact that symbols may or mayn’t be defined is a built-in, conscious, choice of paradigm for the CPP and that’s that.

One thing we can do, once in a while, is to watch the proliferation of symbols. A simple grep regexp finds both #defines and other preprocessor constructs:

grep -e '^\ *\#[^A-Z]*\([A-Z]\+_\)\+' *.h *.c | sort -u

Run on the above code, it yields:

> grep -e '^\ *\#[^A-Z]*\([A-Z]\+_\)\+' cluster.cpp | sort -u
#ifdef CLUSTER_PARAM_DEBUG
#ifdef CLUSTER_PARAM_DEBUG_MODE
 # ifdef CLUSTER_PARAM_DEDUG_MODE

Removing leading white spaces and preprocessor directives using sed:

grep -e '^\ *\#[^A-Z]*\([A-Z]\+_\)\+' *.h *.c | sed s/^\ *\#\ *[a-z]*\ *// | sort -u

Yields an ever better list:

> grep -e '^\ *\#[^A-Z]*\([A-Z]\+_\)\+' cluster.cpp | sed s/^\ *\#\ *[a-z]*\ *// | sort -u
CLUSTER_PARAM_DEBUG
CLUSTER_PARAM_DEBUG_MODE
CLUSTER_PARAM_DEDUG_MODE

(There’s extra work to be done for compound expressions, and also maybe list where they appear in which file, etc., but that’s left as an exercise to the reader.) The next step is to analyze this list, possibly automatically, to detect symbols that are suspiciously close but yet different (as in CLUSTER_PARAM_DEBUG_MODE and CLUSTER_PARAM_DEDUG_MODE, which now show somewhat conspicuously in the list), and detect the symbols that are either undefined in the code or by the build scripts.

*
* *

There are no simple workarounds; but one possibility is to force symbols to be defined all the time and assigned with a truth value (0 or 1). For example:

#ifdef (THIS_SYMBOL)
 #if THIS_SYMBOL==1
   ...code goes here...
 #endif
#else
 #warning THIS_SYMBOL is not defined
#endif

The other workaround is to use as few of these constructs as possible, in all circumstances. The above example with debug print-outs enabled by defined preprocessor symbols is unfortunately quite frequent in code that is still in its inception phase; but it also quite often remains in the later phase of the code, leading to a proliferation of symbols. The correct strategy, in this case, is to use debug levels and better logging. Already, having something like:

double last_error=std::numeric_limits<double>::max();
double this_error=std::numeric_limits<double>::max();

#if DEBUG_TRACE_LEVEL>3
 std::cout << __PRETTY_FUNCTION__
           << ": start nb_colors=" << nb_colors << std::endl;
#endif

is much better than having a private symbol for this part of the code only. Using the __PRETTY_FUNCTION__ (which is GNU-specific, I think) will allow you to grep through the output to find what you’re looking for.

*
* *

I know what you’re thinking: misusing a tool doesn’t make that tool evil. And you’re right, but the way I see it, is that if I spent 5 minutes looking for the reason why a particular piece of code isn’t activated when CLUSTER_PARAM_DEBUG_MODE is defined, then somebody didn’t do his job correctly—which possibly means me as well. I think we have to use tools like the CPP with care, and avoiding whenever possible (even if it requires us to go a bit out of our way) occasions for errors by choosing, when appropriate, different, and better, constructs than those we’re used to.

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: