プログラミング、リファクタリング、そしてすべてにおける究極の疑問: No. 28

2018.11.13
その他インテル® DPC++/C++ コンパイラー特集
C/C++エラー検証実装

この記事は、インテル® デベロッパー・ゾーンに公開されている「The Ultimate Question of Programming, Refactoring, and Everything」(https://software.intel.com/en-us/articles/the-ultimate-question-of-programming-refactoring-and-everything) の日本語参考訳です。

28. 単純な関数を使用できる場合マクロは使用しない

ReactOS* プロジェクトから抜粋した以下のコードについて考えてみます。このコードにはエラーが含まれています。PVS-Studio アナライザーは、次の診断を出力します。

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. (V640 コードの操作ロジックと書式が一致していません。2 つ目の文は常に実行されます。中括弧が不足している可能性があります。)

#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;
}

説明

ここでは、やや長めのコード例を使用しています。簡単な内容なので、理解するのは難しくないでしょう。

このコードは、_tstat64() 関数でファイル情報を取得し、_stat 型の構造体にデータを格納します。データの保存には、stat64_to_stat マクロを使用します。

このマクロの実装が不正です。実行する操作が中括弧 { } でブロックにグループ化されていません。その結果、マクロの最初の文字列のみが条件付き演算子の本体と見なされます。マクロを展開すると、次のようになります。

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

したがって、構造体メンバーのほとんどは情報が取得されたかどうかに関係なくコピーされます。

これはエラーですが、実際にはそれほど致命的なものではありません。初期化されていないメモリーセルが無駄にコピーされるだけです。このケースは運が良かったと言えるでしょう。私は、このような適切に記述されていないマクロに関連した、より重大なエラーを目にしてきました。

正しいコード

最も簡単なバージョンは、マクロに中括弧を追加するだけです。やや優れたバージョンでは、do { …. } while (0) を追加します。そして、マクロと関数の後にセミコロン ‘;’ を追加します。

#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)

推奨事項

マクロは私のお気に入りとは言い難いものの、マクロなしでは、特に C ではコーディングすることができません。そのため、できるだけ使用しないようにしています。皆さんも使用し過ぎないことを推奨します。私がマクロに反対する理由は 3 つあります。

  • コードのデバッグが困難になります。
  • ミスをしやすくなります。
  • コードが理解しづらくなります (特に、あるマクロから別のマクロを使用する場合)。

このほかにも、マクロに関連するさまざまなエラーがあります。私が示したコード例は、マクロが不要な場合もあることを明確にしています。私は、オリジナルコードを記述したプログラマーがなぜ単純な関数を使用しなかったのか理解できません。マクロに対する関数のメリットは次のとおりです。

  • コードが単純です。追加のコードを記述したり、\ 記号を揃える必要がありません。
  • コードの信頼性が高まります (例示したエラーは、関数では不可能です)。

考えられる唯一のデメリットは、最適化です。皆さんもご存じのように、関数は呼び出されます。しかし、これは重大な問題ではありません。

仮に非常に重大な問題であったとして、最適化について考えてみます。まず、inline と言う素晴らしいキーワードがあり、これを使用することができます。次に、関数を static として宣言することが適切でしょう。これで、コンパイラーはこの関数をビルドし、本体を分割することはないでしょう。

コンパイラーが非常に賢くなったため、プログラマーは何も心配する必要はありません。inline/static なしで関数を記述しても、コンパイラーはビルドする価値があると判断すれば、それをビルドします。しかし、そのような詳細を気にする必要はありません。単純で分かりやすいコードを記述することのほうが重要であり、より多くの利点がもたらされます。

私の考えでは、このコードは次のように記述すべきです。

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;
}

このコードはさらに改善できます。例えば、C++ では、ポインターではなく、参照渡しの方が良いでしょう。事前チェックのないポインターは、美しくありません。しかし、それはまた別のトピックなので、ここでは取り上げません。

< 前へ 目次 次へ >

コンパイラーの最適化に関する詳細は、最適化に関する注意事項を参照してください。

タイトルとURLをコピーしました