Table-style formatting

Fragment taken from the ReactOS project (open-source operating system compatible with Windows). The error is detected by the following diagnostic: V560 A part of conditional expression is always true: 10035L.

void adns__querysend_tcp(adns_query qu, struct timeval now) {
  ...
  if (!(errno == EAGAIN || EWOULDBLOCK || 
        errno == EINTR || errno == ENOSPC ||
        errno == ENOBUFS || errno == ENOMEM)) {
  ...
}

Explanation

The code sample given above is small and you can easily spot the error in it. But when dealing with real-life code, bugs are often very hard to notice. Reading code like that, you tend to unconsciously skip blocks of similar comparisons and go on to the next fragment.

скачанные файлы (2)

The reason why it happens, has to do with the fact that conditions are poorly formatted and you don’t feel like paying too much attention to them because it requires certain effort, and we assume that since the checks are similar, there are hardly any mistakes in the condition and everything should be fine.

One of the ways out is formatting the code as a table.

So here “errno ==” is missing in one of the checks. It results in the condition always being true as the EWOULDBLOCK is not equal to zero.

Correct code

if (!(errno == EAGAIN || errno == EWOULDBLOCK || 
      errno == EINTR || errno == ENOSPC ||
      errno == ENOBUFS || errno == ENOMEM)) {

Recommendation

For a start, here’s a version of this code formatted in the simplest “table” style. We don’t like it actually.

if (!(errno == EAGAIN  || EWOULDBLOCK     || 
      errno == EINTR   || errno == ENOSPC ||
      errno == ENOBUFS || errno == ENOMEM)) {

It’s better now, but not quite.

There are two reasons why we don’t like this layout. First, the error is still not much visible; second, you have to insert too many spaces to align the code.

That’s why we need to make two improvements in this formatting style. The first one is we need to use no more than one comparison per line: it makes errors easy to notice. For example:

a == 1 &&
b == 2 &&
c      &&
d == 3 &&

The second improvement is to write operators &&, ||, etc., in a more rational way, i.e. on the left instead of on the right.

See how tedious it is to align code by means of spaces:

x == a          &&
y == bbbbb      &&
z == cccccccccc &&

Writing operators on the left makes it much faster and easier:

   x == a
&& y == bbbbb
&& z == cccccccccc

The code looks a bit odd, but you’ll get used to it very soon.

Let’s combine these two improvements to write our code sample in the new style:

if (!(   errno == EAGAIN
      || EWOULDBLOCK
      || errno == EINTR
      || errno == ENOSPC
      || errno == ENOBUFS
      || errno == ENOMEM)) {

Yes, it’s longer now – yet the error has become clearly seen, too.

We agree that it looks strange, but nevertheless we do recommend this technique. We’ve been using it for half a year now and enjoy it very much.

We don’t find it a problem at all that the code has become longer. We could even write it in a way like this:

const bool error =    errno == EAGAIN
                   || errno == EWOULDBLOCK
                   || errno == EINTR
                   || errno == ENOSPC
                   || errno == ENOBUFS
                   || errno == ENOMEM;
if (!error) {

Feel disappointed with the code being too lengthy and cluttered? So let’s make it a function.

static bool IsInterestingError(int errno)
{
  return    errno == EAGAIN
         || errno == EWOULDBLOCK
         || errno == EINTR
         || errno == ENOSPC
         || errno == ENOBUFS
         || errno == ENOMEM;
}
....
if (!IsInterestingError(errno)) {

Here’s another example from WinDjView project:

inline bool IsValidChar(int c)
{
  return c == 0x9 || 0xA || c == 0xD || 
         c >= 0x20 && c <= 0xD7FF ||
         c >= 0xE000 && c <= 0xFFFD || 
         c >= 0x10000 && c <= 0x10FFFF;
}

The function consists of just a few lines, but it still has an error. The function always returns true. The reason, in the long run, has to do with poor formatting and programmers maintaining the code for many years being unwilling to read it carefully.

Let’s refactor this code in the “table” style, I’d also add some parentheses:

inline bool IsValidChar(int c)
{
  return
       c == 0x9
    || 0xA
    || c == 0xD
    || (c >= 0x20    && c <= 0xD7FF)
    || (c >= 0xE000  && c <= 0xFFFD)
    || (c >= 0x10000 && c <= 0x10FFFF);
}

You don’t have to format your code exactly the way we suggest. The aim of this post is to draw your attention to typos in “chaotically” written code. By arranging it in the “table” style, you can avoid lots of silly typos, and that’s already great. So we hope this post will help you.

Note

Being completely honest, we have to warn you that “table” formatting may sometimes cause harm. Check this example:

inline 
void elxLuminocity(const PixelRGBi& iPixel,
                   LuminanceCell< PixelRGBi >& oCell)
{
  oCell._luminance = 2220*iPixel._red +
                     7067*iPixel._blue +
                     0713*iPixel._green;
  oCell._pixel = iPixel;
}

It’s taken from the eLynx SDK project. The programmer wanted to align the code, so he added 0 before the value 713. Unfortunately, he forgot that 0 being the first digit in a number means that this number is octal.

An array of strings

We hope that the idea about the table formatting of the code is clear, but we just feel like giving couple more examples. Let’s have a look at one more case. By bringing it here, we are saying that the table formatting should be used not only with conditions, but also with other various constructions of a language.

The fragment is taken from Asterisk project. The error is detected by the following diagnostic: V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: “KW_INCLUDES” “KW_JUMP”.

static char *token_equivs1[] =
{
  ....
  "KW_IF",
  "KW_IGNOREPAT",
  "KW_INCLUDES"
  "KW_JUMP",
  "KW_MACRO",
  "KW_PATTERN",
  ....
};

There is a typo here – one comma is forgotten. As a result two strings that have completely different meaning are combined in one, i.e. we actually have:

  ....
  "KW_INCLUDESKW_JUMP",
  ....

The error could be avoided if the programmer used the table formatting. Then, if the comma is omitted, it will be easy to spot.

static char *token_equivs1[] =
{
  ....
  "KW_IF"        ,
  "KW_IGNOREPAT" ,
  "KW_INCLUDES"  ,
  "KW_JUMP"      ,
  "KW_MACRO"     ,
  "KW_PATTERN"   ,
  ....
};

Just like last time, pay attention, that if we put the delimiter to the right (a comma in this case), you have to add a lot of spaces, which is inconvenient. It is especially inconvenient if there is a new long line/phrase: we will have to reformat the entire table.

That’s why we would again recommend formatting the table in the following way:

static char *token_equivs1[] =
{
  ....
  , "KW_IF"
  , "KW_IGNOREPAT"
  , "KW_INCLUDES"
  , "KW_JUMP"
  , "KW_MACRO"
  , "KW_PATTERN"
  ....
};

Now it’s very easy to spot a missing comma and there is no need to use a lot of spaces – the code is beautiful and intuitive. Perhaps this way of formatting may seem unusual, but you quickly get used to it – try it yourself.

Finally, here is our short motto. As a rule, beautiful code is usually correct code.

Written by Andrey Karpov.

This error was found with PVS-Studio static analysis tool.

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