28. Avoid using a macro if you can use a simple function

The fragment is taken from ReactOS project. The code contains an error that PVS-Studio analyzer diagnoses in the following way: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing.

#define stat64_to_stat(buf64, buf)   \
    buf->st_dev   = (buf64)->st_dev;   \
    buf->st_ino   = (buf64)->st_ino;   \
    buf->st_mode  = (buf64)->st_mode;  \
    buf->st_nlink = (buf64)->st_nlink; \
    buf->st_uid   = (buf64)->st_uid;   \
    buf->st_gid   = (buf64)->st_gid;   \
    buf->st_rdev  = (buf64)->st_rdev;  \
    buf->st_size  = (_off_t)(buf64)->st_size;  \
    buf->st_atime = (time_t)(buf64)->st_atime; \
    buf->st_mtime = (time_t)(buf64)->st_mtime; \
    buf->st_ctime = (time_t)(buf64)->st_ctime; \

int CDECL _tstat(const _TCHAR* path, struct _stat * buf)
{
  int ret;
  struct __stat64 buf64;

  ret = _tstat64(path, &buf64);
  if (!ret)
    stat64_to_stat(&buf64, buf);
  return ret;
}

Explanation

This time the code example will be quite lengthy. Fortunately it's rather easy, so it shouldn't be hard to understand.

There was the following idea. If you manage to get file information by means of _tstat64() function, then put these data into the structure of _stat type. We use a stat64_to_stat macro to save data.

The macro is incorrectly implemented. The operations it executes are not grouped in blocks with curly brackets { }. As a result the conditional operator body is only the first string of the macro. If you expand the macro, you'll get the following:

if (!ret)
  buf->st_dev   = (&buf64)->st_dev;
buf->st_ino   = (&buf64)->st_ino;
buf->st_mode  = (&buf64)->st_mode;

Consequently the majority of the structure members are copied regardless of the whether the information was successfully received or not.

This is certainly an error, but in practice it's not a fatal one. The uninitialized memory cells are just copied in vain. We had a bit of luck here. But I've come across more serious errors, connected with such poorly written macros.

Correct code

The easiest variant is just to add curly brackets to the macro. To add do { .... } while (0) is a slightly better variant. Then after the macro and the function you can put a semicolon ';'.

#define stat64_to_stat(buf64, buf)   \
  do { \
    buf->st_dev   = (buf64)->st_dev;   \
    buf->st_ino   = (buf64)->st_ino;   \
    buf->st_mode  = (buf64)->st_mode;  \
    buf->st_nlink = (buf64)->st_nlink; \
    buf->st_uid   = (buf64)->st_uid;   \
    buf->st_gid   = (buf64)->st_gid;   \
    buf->st_rdev  = (buf64)->st_rdev;  \
    buf->st_size  = (_off_t)(buf64)->st_size;  \
    buf->st_atime = (time_t)(buf64)->st_atime; \
    buf->st_mtime = (time_t)(buf64)->st_mtime; \
    buf->st_ctime = (time_t)(buf64)->st_ctime; \
  } while (0)

Recommendation

I cannot say that macros are my favorite. I know there is no way to code without them, especially in C. Nevertheless I try to avoid them if possible, and would like to appeal to you not to overuse them. My macro hostility has three reasons:

• It's hard to debug the code.
• It's much easier to make an error.
• The code gets hard to understand especially when some macros use another macros.

A lot of other errors are connected with macros. The one I've given as an example shows very clearly that sometimes we don't need macros at all. I really cannot grasp the idea of why the authors didn't use a simple function instead. Advantages of a function over a macro:

• The code is simpler. You don't have to spend additional time writing it and, aligning some wacky symbols \.
• The code is more reliable (the error given as an example won't be possible in the code at all)

Concerning the disadvantages, I can only think of optimization. Yes, the function is called but it's not that serious at all.

However, let's suppose that it's a crucial thing to us, and meditate on the topic of optimization. First of all, there is a nice keyword inline which you can use. Secondly, it would be appropriate to declare the function as static. I reckon it can be enough for the compiler to build in this function and not to make a separate body for it.

In point of fact you don't have to worry about it at all, as the compilers have become really smart. Even if you write a function without any inline/static, the compiler will build it in; if it considers that it's worth doing it. But don't really bother going into such details. It's much better to write a simple and understandable code, it'll bring more benefit.

To my mind, the code should be written like this:

static void stat64_to_stat(const struct __stat64 *buf64,
                           struct _stat *buf)
{
  buf->st_dev   = buf64->st_dev;
  buf->st_ino   = buf64->st_ino;
  buf->st_mode  = buf64->st_mode;
  buf->st_nlink = buf64->st_nlink;
  buf->st_uid   = buf64->st_uid;
  buf->st_gid   = buf64->st_gid;
  buf->st_rdev  = buf64->st_rdev;
  buf->st_size  = (_off_t)buf64->st_size;
  buf->st_atime = (time_t)buf64->st_atime;
  buf->st_mtime = (time_t)buf64->st_mtime;
  buf->st_ctime = (time_t)buf64->st_ctime;
}

Actually we can make even more improvements here. In C++ for example, it's better to pass not the pointer, but a reference. The usage of pointers without the preliminary check doesn't really look graceful. But this is a different story, I won't talk about it in a section on macros.