Id SoftwareはPVS-Studioにライセンスされています。 それにもかかわらず、最近ネットワークに投稿されたDoom 3のソースコードを確認することにしました。 結果-エラーはほとんど見つかりませんでしたが、まだ見つかりました。 これは次のように説明できると思います。
Doom3コードの一部が現在使用されており、おそらく、エラーはすでに修正されています。 コードの一部は非推奨であり、使用されていません。 最も可能性が高いのは、疑わしいコードフラグメントが見つかった場所です。
このトピックに興味がある人のために、PVS-Studioアナライザーによって指摘されたコードスニペットを提供します。 いつものように、私はいくつかの警告のみを検討していることを思い出させてください。 プロジェクトの他のセクションでは、プログラムの構造に関する知識が必要であり、私はそれらを研究しませんでした。
Doom 3のソースコードは、 GitHubおよびGPL v3ライセンスの下で会社の公式FTPに公開されてい ます 。 分析には、 PVS-Studio 4.39ツールを使用しました。 プログラムのクラックは、見ないでください。 PVS-Studioのキーやクラックを装ったトロイの木馬を見てきました。 より良い私たちを書いて、しばらくの間トライアルキーを提供します。
フラグメント1.疑わしい状態。
#define BIT（num）（1 <<（num））
const int BUTTON_ATTACK = BIT（0）;
void idTarget_WaitForButton :: Think（void）{
...
if（プレイヤー&&
（！player-> oldButtons＆BUTTON_ATTACK）&&
（player-> usercmd.buttons＆BUTTON_ATTACK））{
...
}
PVS-Studio診断メッセージ：V564「＆」演算子はブール型の値に適用されます。 括弧を含めるのを忘れているか、「&&」演算子を使用することを意図している可能性があります。 ゲームtarget.cpp 257
フラグメント「！Player-> oldButtons＆BUTTON_ATTACK」に注意してください。 ここでは、最下位ビットが0であることを確認したかったのですが、オペレーターの優先順位は 「！」です。 「＆」演算子よりも高い。 これは、条件が次のように機能することを意味します。
（！player-> oldButtons）＆1
すべてのビットがゼロの場合にのみ条件が真であることがわかります。 コードの正しいバージョン：
if（プレイヤー&&
（！（player-> oldButtons＆BUTTON_ATTACK））&&
（player-> usercmd.buttons＆BUTTON_ATTACK））{
フラグメント2.疑わしいサイクル。
void idSurface_Polytope :: FromPlanes（...）
{
...
for（j = 0; j <w.GetNumPoints（）; j ++）{
for（k = 0; k <verts.Num（）; j ++）{
if（verts [k] .xyz.Compare（w [j] .ToVec3（）、
POLYTOPE_VERTEX_EPSILON））{
休憩;
}
}
...
}
...
}
PVS-Studio診断メッセージ：V533「for」演算子内で誤った変数が増分されている可能性があります。 「j」を確認することを検討してください。 idLib surface_polytope.cpp 65
ネストされたループは、「k」ではなく変数「j」をインクリメントします。 変数 'k'はまったく増加しません。 このようなサイクルの結果は予測できません。 コードの正しいバージョン：
for（k = 0; k <verts.Num（）; k ++）{
フラグメント3.別の疑わしいサイクル。
bool idMatX :: IsOrthonormal（const float epsilon）const {
...
for（int i = 0; i <numRows; i ++）{
...
for（i = 1; i <numRows; i ++）{
...
}
if（idMath :: Fabs（sum）> epsilon）{
falseを返します。
}
}
trueを返します。
}
PVS-Studio診断メッセージ：V535変数 'i'は、このループと外側のループに使用されています。 idLib matrix.cpp 3128
ネストされたループは、外側のループと同じ変数を使用して編成されます。 両方のループには停止条件があります：i <numRows。 外側のループは常に1回の反復のみを実行することがわかります。 コードを修正するには、内側のループで別の変数を使用できます。
フラグメント4。未定義の動作。
int idFileSystemLocal :: ListOSFiles（...）
{
...
dir_cache_index =（++ dir_cache_index）％MAX_CACHED_DIRS;
...
}
PVS-Studio診断メッセージ：V567未定義の動作。 'dir_cache_index'変数は、シーケンスポイント間で2回使用されている間に変更されます。 TypeInfo filesystem.cpp 1877
変数 "dir_cache_index"は、シーケンスの同じポイントで 2回変更されます 。 プレフィックスの増分が使用されるという事実は重要ではありません。 理論的には、コンパイラには次の形式のコードを作成する権利があります。
A = dir_cache_index; A = A + 1; B = A％MAX_CACHED_DIRS; dir_cache_index = B; dir_cache_index = A;
もちろん、ほとんどの場合、式は正しく計算されます。 しかし、これを確信することはできません。 結果は、コンパイラのタイプとバージョン、最適化設定の影響を受ける場合があります。 コードの正しいバージョン：
dir_cache_index =（dir_cache_index + 1）％MAX_CACHED_DIRS;
フラグメント5.疑わしいアレイのクリーニング。
void idMegaTexture :: GenerateMegaMipMaps（）{
...
byte * newBlock =（byte *）_ alloca（tileSize）;
...
memset（newBlock、0、sizeof（newBlock））;
...
}
PVS-Studio診断メッセージ：V579 memset関数は、ポインターとそのサイズを引数として受け取ります。 間違いかもしれません。 3番目の引数を調べます。 DoomDLL megatexture.cpp 542
「newBlock」配列の一部のみがゼロで埋められます。 どうやら、これは間違っています。 以前はこのように書かれていたようです。
バイトnewBlock [CONST_ARRAY_SIZE]; ... memset（newBlock、0、sizeof（newBlock））;
その後、要件が変更され、「newBlock」配列のサイズが変更され始めました。 しかし、彼らはそれを掃除する機能を忘れていました。 コードの正しいバージョン：
memset（newBlock、0、tileSize）;
フラグメント6.別の疑わしいアレイのクリーンアップ。
void Sys_GetCurrentMemoryStatus（sysMemoryStats_t＆stats）{
...
memset（＆statex、sizeof（statex）、0）;
...
}
PVS-Studio診断メッセージ：V575「memset」関数は「0」要素を処理します。 3番目の引数を調べます。 DoomDLL win_shared.cpp 177
ここでは、「memset」関数を呼び出すときに、引数が混在しています。 この関数は0バイトをクリアします。 ちなみにこれは非常によくある間違いです。 さまざまなプロジェクトで何度も会いました。
正しい関数呼び出し：
memset（＆statex、0、sizeof（statex））;
フラグメント7.こんにちは、コピーペースト。
void idAASFileLocal :: DeleteClusters（void）{
...
memset（＆portal、0、sizeof（ポータル））;
portals.Append（ポータル）;
memset（＆cluster、0、sizeof（ポータル））;
cluster.Append（クラスター）;
}
PVS-Studio診断メッセージ：V512「memset」関数の呼び出しにより、バッファー「＆cluster」のアンダーフローが発生します。 DoomDLL aasfile.cpp 1312
コードの上部2行と下部2行がどのように見えるかに注目してください。 ほとんどの場合、最後の2行はコピーペーストテクノロジを使用して記述されています。 これによりエラーが発生しました。 ある場所では、「ポータル」という単語を「クラスター」という単語に置き換えるのを忘れていました。 その結果、構造の一部のみがクリアされます。 コードの正しいバージョン：
memset（＆cluster、0、sizeof（クラスター））;
コードには他の「未開発」配列がありますが、それらについて書くのは面白くありません。
フラグメント8.ポインターを使用した疑わしい作業。
void idBrushBSP :: FloodThroughPortals_r（idBrushBSPNode * node、...）
{
...
if（ノード->占有）{
common-> Error（ "FloodThroughPortals_r：ノードはすでに占有されています\ n"）;
}
if（！node）{
common-> Error（ "FloodThroughPortals_r：NULL node \ n"）;
}
...
}
PVS-Studio診断メッセージ：V595 nullptrに対して検証される前に、「ノード」ポインターが使用されました。 チェック行：1421、1424。DoomDLL brushbsp.cpp 1421
最初に、「ノード」ポインターの名前が変更されます：ノード->占有。 そして、突然NULLかどうかがチェックされます。 これは非常に疑わしいコードです。 関数のロジックがわからないため、正しくする方法がわかりません。 たぶんこれを書いてください：
if（ノード&& node->占有）{
フラグメント9.疑わしい文字列形式。
struct gameVersion_s {
gameVersion_s（void）
{
sprintf（文字列、「％s。％d％s％s％s」、
ENGINE_VERSION、BUILD_NUMBER、BUILD_DEBUG、
BUILD_STRING、__ DATE __、__ TIME__）;
}
文字列[256];
} gameVersion;
PVS-Studio診断メッセージ：V576形式が正しくありません。 「sprintf」関数の呼び出し中に、異なる数の実引数が予期されます。 予想：7.現在：8.ゲームsyscvar.cpp 54
引数「__TIME__」がまったく使用されていないことは疑わしいです。
フラグメント10。紛らわしいコード。
繰り返し発見したコードは、私が理解しているように、正しく動作しますが、奇妙に見えます。 そのようなコードの例を1つだけ示します。
static bool R_ClipLineToLight（...、const idPlane frustum [4]、...）
{
...
for（j = 0; j <6; j ++）{
d1 =錐台[j]。距離（p1）;
d2 =錐台[j]。距離（p2）;
...
}
...
}
ヒントとして、プログラマーは配列「frustum」が4つの要素で構成されていると書いています。 ただし、6つの要素が処理されます。 「R_ClipLineToLight」の呼び出しを見ると、6つの要素の配列があります。 つまり、すべてが正しく動作するはずですが、コードはあなたに考えさせます。
他のエラーと欠点は、PVS-Studioアナライザーを起動することで確認できます。 ところで、この機会を利用して、John Carmackに敬意を表し、id SoftwareでPVS-Studioを完全に使用できない欠陥をすぐに修正することを伝えたいと思います。
この欠点は、アナライザーの速度が遅いことです。 同社が使用している大量のソースコードを考えると、これは大きな制限です。 今年リリースされるPVS-Studioバージョン4.50では、Visual C ++プリプロセッサではなくClangをプリプロセッサとして使用できます。 これにより、プロジェクトの検証が大幅に加速されます。 たとえば、Visual C ++のプリプロセッサを使用すると、Doom 3ソースコードは26分でチェックされます。 Clangプリプロセッサを使用する場合-16分で。 ただし、この例はあまり成功していません。 ほとんどのプロジェクトでは、分析速度の向上がはるかに強くなります。
True、デフォルトでは、Visual C ++のプリプロセッサを使用する必要があります。 Clangには、Windowsプラットフォームに関する非互換性と欠陥がまだあります。 したがって、プロジェクトの80％のみが正常に検証されます。