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

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

この記事は、インテル® デベロッパー・ゾーンに公開されている「The Ultimate Question of Programming, Refactoring, and Everything」の日本語参考訳です。

23. 文字列リテラルの長さを自動的に評価する

OpenSSL* ライブラリーから抜粋した以下のコードについて考えてみます。このエラーは、次の PVS-Studio 診断によって検出されます。

V666 Consider inspecting the third argument of the function ‘strncmp’. It is possible that the value does not correspond with the length of a string which was passed with the second argument. (V666 関数 ‘strncmp’ の第 3 引数を確認してください。値が、第 2 引数で渡された文字列の長さに対応していない可能性があります。)

if (!strncmp(vstart, "ASCII", 5))
  arg->format = ASN1_GEN_FORMAT_ASCII;
else if (!strncmp(vstart, "UTF8", 4))
  arg->format = ASN1_GEN_FORMAT_UTF8;
else if (!strncmp(vstart, "HEX", 3))
  arg->format = ASN1_GEN_FORMAT_HEX;
else if (!strncmp(vstart, "BITLIST", 3))
  arg->format = ASN1_GEN_FORMAT_BITLIST;
else
  ....

説明

マジックナンバーを使用しないようにするのは非常に困難です。また、0、1、-1、10 のような定数を排除するのも非常に非合理的です。このような定数に名前を付けることは難しく、もし名前を付けても多くの場合コードが複雑になります。

しかし、マジックナンバーを減らすのには役立ちます。例えば、文字列リテラルの長さを定義するマジックナンバーを排除するのは良いことです。

上記のコードについて考えてみます。このコードは、コピー & ペーストを使用して記述された可能性が高く、プログラマーは、次の行をコピーしたと考えられます。

else if (!strncmp(vstart, "HEX", 3))

そして、”HEX” を “BITLIST” に置き換えた後、3 を 7 に変更し忘れたのでしょう。その結果、文字列が “BITLIST” ではなく、”BIT” のみと比較されています。これは、重大なエラーではないかもしれませんが、エラーであることに変わりはありません。

このケースでは、コードがコピー & ペーストを使用して記述されていることが過ちです。さらに、文字列の長さがマジック定数で定義されています。ときどき、プログラマーのタイプミスや不注意により、文字列の長さが記号の数と一致しない、このようなエラー (http://www.viva64.com/en/examples/V666/) に遭遇することがあります。つまり、これはよくあるエラーであり、何とかする必要があります。このようなエラーを回避する方法について詳しく見てみましょう。

正しいコード

strncmp() 呼び出しを strcmp() に置き換えるだけで十分に見えます。これにより、マジック定数がなくなります。

else if (!strcmp(vstart, "HEX"))

残念ながら、これはコードのロジックを変えてしまいます。strncmp() 関数は文字列の先頭が “HEX” であるかチェックします。一方、strcmp() 関数は文字列が等しいかチェックします。この 2 つのチェックは異なります。

この問題を解決する最も簡単な方法は、定数を変更することです。

else if (!strncmp(vstart, "BITLIST", 7))
  arg->format = ASN1_GEN_FORMAT_BITLIST;

これは正しいコードですが、まだマジック定数 7 が使用されているため、最適なコードとは言えません。そのため、私は異なる方法を推奨します。

推奨事項

このようなエラーは、コードで文字列の長さを明示的に評価することで回避できます。最も簡単な方法は、strlen() 関数を使用することです。

else if (!strncmp(vstart, "BITLIST", strlen("BITLIST")))

そうすることで、文字列の 1 つを修正し忘れた場合、不一致を見つけやすくなります。

else if (!strncmp(vstart, "BITLIST", strlen("HEX")))

ただし、この方法では、以下の 2 点に注意する必要があります。

  1. コンパイラーが strlen() 呼び出しを最適化して定数に置き換えないという保証はありません。
  2. 文字列リテラルを 2 回記述する必要があります。見た目が美しくなく、またミスを引き起こす可能性もあります。

1 つ目は、コンパイル時にリテラルの長さに特殊な構造を使用することで対応できます。例えば、次のようなマクロを使用します。

#define StrLiteralLen(arg) ((sizeof(arg) / sizeof(arg[0])) - 1)
....
else if (!strncmp(vstart, "BITLIST", StrLiteralLen("BITLIST")))

ただし、このマクロは危険を伴います。リファクタリング・プロセス中にコードは次のように変換されます。

const char *StringA = "BITLIST"; 
if (!strncmp(vstart, StringA, StrLiteralLen(StringA)))

このケースでは、StrLiteralLen マクロは無意味な結果を返します。ポインターサイズ (4 または 8 バイト) に応じて、値 3 または 7 になります。C++ では、より複雑な手法を使用することで、これを回避できます。

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define StrLiteralLen(str) (sizeof(ArraySizeHelper(str)) - 1)

これで、StrLiteralLen マクロの引数が単純なポインターの場合、コードをコンパイルできなくなります。

2 つ目の注意点 (文字列リテラルの重複) について、C プログラマーに対する推奨はありません。特別なマクロを記述することができますが、個人的にはお勧めしません。私はマクロの愛好者ではないので、何を推奨したら良いのか分かりません。

C++ では、素晴らしい手法があります。テンプレート関数を利用して、2 つ目の注意点だけでなく、1 つ目の注意点にもより洗練した方法で対応できます。さまざまな記述方法がありますが、一般に次のようなコードになります。

template<typename T, size_t N>
int mystrncmp(const T *a, const T (&b)[N])
{
  return _tcsnccmp(a, b, N - 1);
}

これで、文字列リテラルの使用が 1 回になります。文字列リテラルの長さは、コンパイル時に評価されます。誤って単純なポインターを関数に渡して、文字列の長さを評価することはできません。すぐに実践してみてください。

要約: 文字列を操作する場合、マジックナンバーは使用しないようにします。マクロとテンプレート関数を使用することで、コードの安全性が高まるだけでなく、美しく、簡潔になります。

例えば、関数 strcpy_s () の宣言について考えてみます。

errno_t strcpy_s(
   char *strDestination,
   size_t numberOfElements,
   const char *strSource 
);
template <size_t size>
errno_t strcpy_s(
   char (&strDestination)[size],
   const char *strSource 
); // C++ only

1 つ目は、C 言語、または事前にバッファーサイズが不明な場合です。スタック上に作成されたバッファーを使用する場合は、C++ の 2 つ目のバージョンを使用できます。

char str[BUF_SIZE];
strcpy_s(str, "foo");

マジックナンバーも、バッファーサイズの評価も含まれていません。簡潔で素晴らしいコードです。

< 前へ 目次 次へ >

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

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