UnicornsがRTSに侵入：OpenRAソースコードの分析

この記事では、PVS-Studio静的アナライザーを使用してOpenRAプロジェクトを確認する方法について説明します。OpenRAとは何ですか？これは、リアルタイムの戦略ゲームを作成するためのオープンソースのゲームエンジンです。この記事では、分析がどのように実行されたか、プロジェクト自体のどの機能が発見されたか、PVS-Studioがどのような興味深いトリガーを提供したかについて説明します。そしてもちろん、ここでは、プロジェクトの検証プロセスをより快適にするアナライザーの機能のいくつかを検討します。

OpenRA

レビュー用に選択されたプロジェクトは、Command＆Conquer：RedAlertのようなゲームスタイルのRTSゲームエンジンです。詳細については、ウェブサイトをご覧ください。ソースコードはC＃で記述されており、リポジトリで表示および使用できます。



OpenRAは、3つの理由でレビューのために選択されました。まず、多くの人が興味を持っているようです。いずれにせよ、リポジトリは8000を超える星を収集しているため、これはGitHubの住民に適用されます。次に、OpenRAコードベースには1285個のファイルが含まれています。通常、この量は、うまくいけば、それらの中で興味深いトリガーを見つけるのに十分です。そして第三に...ゲームエンジンはかっこいいです。

余分なポジティブ

私はPVS-Studioを使用してOpenRAを分析し、最初はその結果に触発されました。





非常に多くの高警告の中で、私は確かに非常に異なる応答をたくさん見つけることができると決めました。そしてもちろん、彼らに基づいて、私は最もクールで最も興味深い記事を書きます。しかし、それはありませんでした！



警告自体を見るだけで、すべてがすぐに実行されました。 1306の高警告のうち、1,277がV3144診断に関連付けられていました。 「このファイルはcopyleftライセンスでマークされているため、派生ソースコードを開く必要があります」などのメッセージが表示されます。この診断については、こちらで詳しく説明しています。



OpenRAはすでにオープンソースプロジェクトであるため、明らかに、そのような計画の仕組みは私にはまったく興味がありませんでした。したがって、ログの残りの部分の表示を妨げないように、非表示にする必要がありました。Visual Studioのプラグインを使用していたので、簡単に行えました。V3144アラートの1つを右クリックし、開いたメニューで[すべてのV3144エラーを非表示]を選択する必要がありました。





アナライザーオプションの[検出可能なエラー（C＃）]セクションに移動して、ログに表示する警告を選択することもできます。





Visual Studio 2019のプラグインを使用してそれらに移動するには、トップメニューの[拡張機能]-> [PVS-スタジオ]-> [オプション]をクリックする必要があります。

試験結果

V3144トリガーがフィルターで除外された 後、ログに記録される警告は大幅に少なくなります。





それにもかかわらず、私たちはそれらの中で興味深い瞬間を見つけることができました。

無意味な条件

かなりの数のトリガーが不要なチェックを示していました。通常、人々はそのようなコードを意図的に記述しないため、これはエラーを示している可能性があります。ただし、OpenRAでは、これらの不要な条件が意図的に追加されたように見えることがよくあります。例えば：

public virtual void Tick()
{
  ....

  Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
  if (!Active)
    return;

  if (Active)
  {
    ....
  }
}

アナライザーの警告：V3022式「アクティブ」は常に真です。 SupportPowerManager.cs 206



PVS-Studioは、Activeがfalseの場合、実行が到達しないため、2番目のチェックは無意味であると正しく指摘しています。ここは間違いかもしれませんが、わざと書いたものだと思います。何のために？さて、なぜですか？



おそらく、私たちの前にある種の一時的な解決策があり、その改訂は「後で」残されています。そのような場合、アナライザーが開発者にそのような欠陥を思い出させるのは非常に便利です。



「万が一に備えて」もう1つのチェックを見てみましょう。

Pair<string, bool>[] MakeComponents(string text)
{
  ....

  if (highlightStart > 0 && highlightEnd > highlightStart)  // <=
  {
    if (highlightStart > 0)                                 // <=
    {
      // Normal line segment before highlight
      var lineNormal = line.Substring(0, highlightStart);
      components.Add(Pair.New(lineNormal, false));
    }
  
    // Highlight line segment
    var lineHighlight = line.Substring(
      highlightStart + 1, 
      highlightEnd - highlightStart – 1
    );
    components.Add(Pair.New(lineHighlight, true));
    line = line.Substring(highlightEnd + 1);
  }
  else
  {
    // Final normal line segment
    components.Add(Pair.New(line, false));
    break;
  }
  ....
}

アナライザーの警告：V3022式 'highlightStart> 0'は常にtrueです。 LabelWithHighlightWidget.cs 54



繰り返しますが、再チェックが完全に無意味であることは明らかです。ハイライトスタート値は2回チェックされ、隣接する行でチェックされます。エラー？条件の1つで、テスト用に間違った変数が選択された可能性があります。いずれにせよ、これが何であるかを確実に言うのは難しいです。 1つ明らかなことは、コードを調査して修正する必要があるか、何らかの理由で追加の検証が必要な場合は説明を残す必要があるということです。



別の同様のポイントがあります：

public static void ButtonPrompt(....)
{
  ....
  var cancelButton = prompt.GetOrNull<ButtonWidget>(
    "CANCEL_BUTTON"
  );
  ....

  if (onCancel != null && cancelButton != null)
  {
    cancelButton.Visible = true;
    cancelButton.Bounds.Y += headerHeight;
    cancelButton.OnClick = () =>
    {
      Ui.CloseWindow();
      if (onCancel != null)
        onCancel();
    };

    if (!string.IsNullOrEmpty(cancelText) && cancelButton != null)
      cancelButton.GetText = () => cancelText;
  }
  ....
}

アナライザーの警告：V3063条件式の一部は、評価されると常にtrueになります：cancelButton！= Null。GetOrNullメソッドによって返される値がこの変数に書き込まれるため、ConfirmationDialogs.cs



78cancelButtonは実際にnullになる可能性があります。ただし、条件ステートメントの本文でcancelButtonがnullに変わることはないと考えるのは論理的です。それにもかかわらず、まだチェックがあります。外部条件に注意を払わないと、一般に奇妙な状況が発生します。最初に変数のプロパティにアクセスし、次に開発者が確認することにしました-それはまだnullかどうか？



最初は、プロジェクトが「== "演算子」のオーバーロードに関連する特定のロジックを使用している可能性があると想定しました。私の意見では、プロジェクトの参照タイプに類似したものを実装することは物議を醸すアイデアです。それでも、異常な動作により、他の開発者がコードを理解するのが難しくなります。同時に、そのようなトリックを免れない状況を想像するのは難しいです。場合によっては、これが便利な解決策になる可能性があります。



たとえば、Unityゲームエンジンでは、「== "」演算子がUnityEngine.Objectクラスに対して再定義されます。リンクで入手可能な公式ドキュメントは、このクラスのインスタンスをnullと比較することを示しています通常どおり動作しません。まあ、確かに開発者はそのような珍しいロジックを実装する理由がありました。



OpenRAでそのようなものは見つかりませんでした:)。したがって、以前に検討されたnullのチェックに意味がある場合、それは別のもので構成されます。



PVS-Studioは、さらにいくつかの同様の瞬間を検出できましたが、ここにすべてをリストする必要はありません。同じポジティブを見るのはまだ退屈です。幸いなことに（またはそうではなく）、アナライザーは他の奇妙なことも見つけることができました。

到達不能なコード

void IResolveOrder.ResolveOrder(Actor self, Order order)
{
  ....
  if (!order.Queued || currentTransform == null)
    return;
  
  if (!order.Queued && currentTransform.NextActivity != null)
    currentTransform.NextActivity.Cancel(self);

  ....
}

アナライザーの警告：V3022式 '！Order.Queued && currentTransform.NextActivity！= Null'は常にfalseです。 TransformsIntoTransforms.cs44



ここでも無意味なチェックがあります。ただし、前のものとは異なり、ここでは追加の条件だけでなく、実際に達成できないコードを示します。以前に考えられていた常に真のチェックは、実際にはプログラムの動作に影響を与えませんでした。それらはコードから削除することも、残すこともできます-何も変更されません。



ここで、奇妙なチェックは、コードの一部が実行されていないという事実につながります。同時に、ここで修正としてどのような変更を行うべきかを推測することは困難です。最も単純で最も快適なケースでは、到達不能なコードは単に実行されるべきではありません。そうすれば間違いはありません。しかし、プログラマーが美のためだけに意図的にこの行を書いたのではないかと思います。

コンストラクターの初期化されていない変数

public class CursorSequence
{
  ....
  public readonly ISpriteFrame[] Frames;

  public CursorSequence(
    FrameCache cache, 
    string name, 
    string cursorSrc, 
    string palette, 
    MiniYaml info
  )
  {
    var d = info.ToDictionary();

    Start = Exts.ParseIntegerInvariant(d["Start"].Value);
    Palette = palette;
    Name = name;

    if (
      (d.ContainsKey("Length") && d["Length"].Value == "*") || 
      (d.ContainsKey("End") && d["End"].Value == "*")
    ) 
      Length = Frames.Length - Start;
    else if (d.ContainsKey("Length"))
      Length = Exts.ParseIntegerInvariant(d["Length"].Value);
    else if (d.ContainsKey("End"))
      Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
    else
      Length = 1;

    Frames = cache[cursorSrc]
      .Skip(Start)
      .Take(Length)
      .ToArray();

    ....
  }
}

アナライザーの警告：V3128「フレーム」フィールドは、コンストラクターで初期化される前に使用されます。CursorSequence.cs35



非常に不快な瞬間。初期化されていない変数からLengthプロパティの値を取得しようとすると、必然的にNullReferenceExceptionがスローされます。通常の状況では、このようなエラーが見過ごされることはほとんどありませんが、クラスのインスタンスを作成できないことは簡単に明らかになります。ただし、ここでは、条件が次の場合にのみ例外がスローされます

(d.ContainsKey("Length") && d["Length"].Value == "*") || 
(d.ContainsKey("End") && d["End"].Value == "*")

本当になります。



すべてがうまく機能するようにコードを修正する必要があるかどうかを判断するのは難しいです。関数は次のようになっているはずです。

public CursorSequence(....)
{
  var d = info.ToDictionary();

  Start = Exts.ParseIntegerInvariant(d["Start"].Value);
  Palette = palette;
  Name = name;
  ISpriteFrame[] currentCache = cache[cursorSrc];
    
  if (
    (d.ContainsKey("Length") && d["Length"].Value == "*") || 
    (d.ContainsKey("End") && d["End"].Value == "*")
  ) 
    Length = currentCache.Length - Start;
  else if (d.ContainsKey("Length"))
    Length = Exts.ParseIntegerInvariant(d["Length"].Value);
  else if (d.ContainsKey("End"))
    Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
  else
    Length = 1;

  Frames = currentCache
    .Skip(Start)
    .Take(Length)
    .ToArray();

  ....
}

このバージョンでは、指定された問題はありませんが、開発者だけが元のアイデアにどれだけ対応しているかを言うことができます。

潜在的なタイプミス

public void Resize(int width, int height)
{
  var oldMapTiles = Tiles;
  var oldMapResources = Resources;
  var oldMapHeight = Height;
  var oldMapRamp = Ramp;
  var newSize = new Size(width, height);

  ....
  Tiles = CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
  Resources = CellLayer.Resize(
    oldMapResources,
    newSize,
    oldMapResources[MPos.Zero]
  );
  Height = CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
  Ramp = CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);  
  ....
}

アナライザーの警告：V31272つの類似したコードフラグメントが見つかりました。おそらくこれはタイプミスであり、「oldMapHeight」の代わりに「oldMapRamp」変数を使用する必要があります。Map.cs964



アナライザーは、関数への引数の受け渡しに関連する疑わしい瞬間を検出しました。呼び出しを個別に見てみましょう。

CellLayer.Resize(oldMapTiles,     newSize, oldMapTiles[MPos.Zero]);
CellLayer.Resize(oldMapResources, newSize, oldMapResources[MPos.Zero]);
CellLayer.Resize(oldMapHeight,    newSize, oldMapHeight[MPos.Zero]);
CellLayer.Resize(oldMapRamp,      newSize, oldMapHeight[MPos.Zero]);

奇妙なことに、最後の呼び出しはoldMapRampではなくoldMapHeightを渡します。もちろん、そのようなすべてのケースが実際にエラーであるわけではありません。ここにすべてが正しく書かれている可能性は十分にあります。しかし、あなたはこの場所が珍しいように見えることを認めなければなりません。私はここで間違いが実際に行われたと信じる傾向があります。



同僚からのメモ-AndreyKarpov。そして、私はこのコードに奇妙なことは何も見ていません。これは古典的な最後の行のエラーです！



それでもここにエラーがない場合は、説明を追加する価値があります。結局のところ、瞬間が間違いのように見える場合、誰かが間違いなくそれを修正したいと思うでしょう。

真実、真実、そして真実以外の何物でもない

プロジェクトには非常に特殊なメソッドが含まれており、その戻り値はboolタイプです。それらの特徴は、どのような条件下でも真に戻るという事実にあります。例えば：

static bool State(
  S server, 
  Connection conn, 
  Session.Client client, 
  string s
)
{
  var state = Session.ClientState.Invalid;
  if (!Enum<Session.ClientState>.TryParse(s, false, out state))
  {
    server.SendOrderTo(conn, "Message", "Malformed state command");
    return true;
  }

  client.State = state;

  Log.Write(
    "server", 
    "Player @{0} is {1}",
    conn.Socket.RemoteEndPoint, 
    client.State
  );

  server.SyncLobbyClients();

  CheckAutoStart(server);

  return true;
}

アナライザーの警告：V3009このメソッドが常に同じ値の「true」を返すのは奇妙なことです。LobbyCommands.cs 123



このコードは大丈夫ですか？間違いはありますか？とても奇妙に見えます。開発者がvoidを使用しなかったのはなぜですか？



アナライザーがそのような場所を奇妙だと見なすことは驚くべきことではありませんが、それでもプログラマーが実際にこのように書く理由があったことを認めなければなりません。どれ？



このメソッドがどこで呼び出され、常に真の戻り値が使用されているかを確認することにしました。同じクラス内にそれへの参照が1つしかないことが判明しました-commandHandlers辞書にはタイプがあります

IDictionary<string, Func<S, Connection, Session.Client, string, bool>>

初期化中に、値が追加されます

{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}

等



静的型付けによって問題が発生するというまれな（信じたい）ケースが表示されます。結局のところ、値が異なる署名を持つ関数になる辞書を作成することは...少なくとも問題があります。commandHandlersは、InterpretCommandメソッドでのみ使用されます。

public bool InterpretCommand(
  S server, Connection conn, Session.Client client, string cmd
)
{
  if (
    server == null || 
    conn == null || 
    client == null || 
    !ValidateCommand(server, conn, client, cmd)
  )  return false;

  var cmdName = cmd.Split(' ').First();
  var cmdValue = cmd.Split(' ').Skip(1).JoinWith(" ");

  Func<S, Connection, Session.Client, string, bool> a;
  if (!commandHandlers.TryGetValue(cmdName, out a))
    return false;

  return a(server, conn, client, cmdValue);
}

どうやら、開発者の目標は、実行された特定の操作の文字列を一致させる普遍的な能力でした。彼が選んだ方法はそれだけではないと思いますが、そのような状況でもっと便利で正しいものを提供するのはそれほど簡単ではありません。特に、ある種のダイナミックまたは類似のものを使用しない場合。このテーマについて何かアイデアがあれば、コメントを残してください。この問題を解決するためのさまざまなオプションを検討することは私にとって興味深いことです。このクラスの常にtrueのメソッドに



関連付けられている警告は、おそらくfalseであることがわかります。それでも...あなたを怖がらせるのはこの「最も可能性が高い」ことです。その中には確かに間違いがあるかもしれないので、あなたはそのようなことに注意する必要があります。



このようなすべての陽性は注意深くチェックし、必要に応じて偽としてマークする必要があります。これは非常に簡単に行われます。アナライザーによって示された場所に特別なコメントを残す必要があります。

static bool State(....) //-V3009

別の方法があります。falseとしてマークする必要があるポジティブを選択し、コンテキストメニューで[選択したメッセージをFalseアラームとしてマークする]をクリックします。





このトピックの詳細については、ドキュメントをご覧ください。

nullの追加チェック？

static bool SyncLobby(....)
{
  if (!client.IsAdmin)
  {
    server.SendOrderTo(conn, "Message", "Only the host can set lobby info");
    return true;
  }

  var lobbyInfo = Session.Deserialize(s); 
  if (lobbyInfo == null)                    // <=
  {
    server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
    return true;
  }

  server.LobbyInfo = lobbyInfo;

  server.SyncLobbyInfo();

  return true;
}

アナライザーの警告：V3022式 'lobbyInfo == null'は常にfalseです。 LobbyCommands.cs851



常にtrueを返す別のメソッド。ただし、今回は別のタイプのトリガーを調べています。これが単なる冗長なコードであるという事実からはほど遠いので、そのようなことを十分に注意深く研究する必要があります。しかし、まず最初に。Deserialize



メソッドがnullを返すことはありません。コードを確認することで、これを簡単に確認できます。

public static Session Deserialize(string data)
{
  try
  {
    var session = new Session();
    ....
    return session;
  }
  catch (YamlException)
  {
    throw new YamlException(....);
  }
  catch (InvalidOperationException)
  {
    throw new YamlException(....);
  }
}

読みやすくするために、メソッドのソースコードを短くしました。リンクをたどると完全に見ることができます。さて、または私の言葉を借りれば、ここのセッション変数はいかなる状況でもnullになりません。



一番下に何が見えますか？Deserializeはnullを返しません。何か問題が発生した場合、例外がスローされます。呼び出し後にnullのチェックを作成した開発者は、考え方が異なるようでした。ほとんどの場合、例外的な状況では、SyncLobbyメソッドは現在実行されているコードを実行する必要があります...はい、lobbyInfoがnullになることはないため、実行されることはありません。

if (lobbyInfo == null)
{
  server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
  return true;
}

私の代わりに、この「余分」のチェックを、あなたはまだ使用する必要があると信じているのtry -キャッチ。または、反対側から行って、TryDeserializeを記述します。これは、例外の場合にnullを返します。

考えられるNullReferenceException

public ConnectionSwitchModLogic(....)
{
  ....
  var logo = panel.GetOrNull<RGBASpriteWidget>("MOD_ICON");
  if (logo != null)
  {
    logo.GetSprite = () =>
    {
      ....
    };
  }

  if (logo != null && mod.Icon == null)                    // <=
  {
    // Hide the logo and center just the text
    if (title != null)
    title.Bounds.X = logo.Bounds.Left;

    if (version != null)
      version.Bounds.X = logo.Bounds.X;
    width -= logo.Bounds.Width;
  }
  else
  {
    // Add an equal logo margin on the right of the text
    width += logo.Bounds.Width;                           // <=
  }
  ....
}

アナライザーの警告：V3125'logo 'オブジェクトは、nullに対して検証された後に使用されました。チェック行：236、222。ConnectionLogic.cs 236



ここに100％のエラーがあると何かが教えてくれます。GetOrNullメソッドは、おそらく実際にnull参照を返す可能性があるため、私たちの前に「余分な」チェックは絶対にありません。ロゴがnullの場合はどうなりますか？Boundsプロパティを呼び出すと例外がスローされますが、これは明らかに開発者の計画には含まれていませんでした。



おそらく、フラグメントは次のように書き直す必要があります。

if (logo != null)
{
  if (mod.Icon == null)
  {
    // Hide the logo and center just the text
    if (title != null)
    title.Bounds.X = logo.Bounds.Left;

    if (version != null)
      version.Bounds.X = logo.Bounds.X;
    width -= logo.Bounds.Width;
  }
  else
  {
    // Add an equal logo margin on the right of the text
    width += logo.Bounds.Width;
  }
}

このオプションは理解するのに十分簡単ですが、追加のネストはあまりクールに見えません。より容量の大きいソリューションとして、null条件演算子を使用できます。

// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=

私はトップフィックスの方が好きだということに注意してください。それを読むのは楽しいですし、疑問は生じません。しかし、一部の開発者は簡潔さを高く評価しているので、2番目のオプションも与えることにしました:)。

多分それは結局OrDefaultですか？

public MapEditorLogic(....)
{
  var editorViewport = widget.Get<EditorViewportControllerWidget>("MAP_EDITOR");

  var gridButton = widget.GetOrNull<ButtonWidget>("GRID_BUTTON");
  var terrainGeometryTrait = world.WorldActor.Trait<TerrainGeometryOverlay>();

  if (gridButton != null && terrainGeometryTrait != null) // <=
  {
    ....
  }

  var copypasteButton = widget.GetOrNull<ButtonWidget>("COPYPASTE_BUTTON");
  if (copypasteButton != null)
  {
    ....
  }

  var copyFilterDropdown = widget.Get<DropDownButtonWidget>(....);
  copyFilterDropdown.OnMouseDown = _ =>
  {
    copyFilterDropdown.RemovePanel();
    copyFilterDropdown.AttachPanel(CreateCategoriesPanel());
  };

  var coordinateLabel = widget.GetOrNull<LabelWidget>("COORDINATE_LABEL");
  if (coordinateLabel != null)
  {
    ....
  }

  ....
}

アナライザーの警告：V3063条件式の一部は、評価されると常にtrueになります：terrainGeometryTrait！= Null。 MapEditorLogic.cs35



このスニペットを分析してみましょう。WidgetクラスのGetOrNullメソッドが使用されるたびに、nullがチェックされることに注意してください。ただし、Getが使用されている場合、検証は行われません。これは論理的です-Getメソッドはnullを返しません：

public T Get<T>(string id) where T : Widget
{
  var t = GetOrNull<T>(id);
  if (t == null)
    throw new InvalidOperationException(....);
  return t;
}

要素が見つからない場合は、例外がスローされます。これは妥当な動作です。同時に、論理的なオプションは、GetOrNullメソッドによって返された値がnull参照と等しいかどうかを確認することです。



上記のコードでは、Traitメソッドによって返される値がnullであるかどうかがチェックされます。実際、Traitメソッド内では、GetはTraitDictionaryクラスから呼び出されます。

public T Trait<T>()
{
  return World.TraitDict.Get<T>(this);
}

このGetの動作は、前に説明したものとは異なる可能性がありますか？しかし、クラスは異なります。確認しよう：

public T Get<T>(Actor actor)
{
  CheckDestroyed(actor);
  return InnerGet<T>().Get(actor);
}

InnerGet メソッドは、TraitContainer <T>のインスタンスを返します。このクラスでのGetの実装は、WidgetクラスのGetと非常によく似ています。

public T Get(Actor actor)
{
  var result = GetOrDefault(actor);
  if (result == null)
    throw new InvalidOperationException(....);
  return result;
}

主な類似点は、ここでもnullが返されないことです。何か問題が発生した場合、InvalidOperationExceptionも同様にスローされます。したがって、Traitメソッドは同じように動作します。



はい、ここに追加のチェックがあるだけで、何の影響もありません。奇妙に見えるかもしれませんが、そのようなコードが読者を大いに混乱させるとは言えません。ただし、ここでチェックが必要な場合は、予期せず例外がスローされることがあります。悲しいです。



この時点で、TraitOrNullを呼び出す方が適切なようです。ただし、そのような方法はありません:)。しかし、GetOrNullに類似したTraitOrDefaultがありますこの場合。Get



メソッドに関連する別の同様のポイントがあります：

public AssetBrowserLogic(....)
{
  ....
  frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
  if (frameSlider != null)
  {
    ....
  }
  ....
}

アナライザーの警告：V3022式 'frameSlider！= Null'は常にtrueです。AssetBrowserLogic.cs 128上記



のコードと同様に、ここで問題が発生しています。チェックは本当に不要であるか、Getの代わりにGetOrNullを呼び出す必要があります。

割り当てを失った

public SpawnSelectorTooltipLogic(....)
{
  ....
  var textWidth = ownerFont.Measure(labelText).X;
  if (textWidth != cachedWidth)
  {
    label.Bounds.Width = textWidth;
    widget.Bounds.Width = 2 * label.Bounds.X + textWidth; // <=
  }

  widget.Bounds.Width = Math.Max(                         // <=
    teamWidth + 2 * labelMargin, 
    label.Bounds.Right + labelMargin
  );
  team.Bounds.Width = widget.Bounds.Width;
  ....
}

アナライザーの警告：V3008'widget.Bounds.Width '変数には、2回連続して値が割り当てられます。おそらくこれは間違いです。チェックライン：78、75 SpawnSelectorTooltipLogic.cs 78は、



あればということらしい！textWidth = CachedWidth条件がある真の、いくつかの特定の値が書き込まれるまでwidget.Bounds.Width。ただし、この条件が真であるかどうかに関係なく、以下の割り当ては文字列を奪います

widget.Bounds.Width = 2 * label.Bounds.X + textWidth;

あらゆる意味で。彼らは単に他のものをここに置くのを忘れた可能性があります：

if (textWidth != cachedWidth)
{
  label.Bounds.Width = textWidth;
  widget.Bounds.Width = 2 * label.Bounds.X + textWidth;
}
else
{
  widget.Bounds.Width = Math.Max(
    teamWidth + 2 * labelMargin,
    label.Bounds.Right + labelMargin
  );
}

デフォルト値の確認

public void DisguiseAs(Actor target)
{
  ....
  var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
  AsPlayer = tooltip.Owner;
  AsActor = target.Info;
  AsTooltipInfo = tooltip.TooltipInfo;
  ....
}

アナライザーの警告：V3146「tooltip」のnull逆参照の可能性。 'FirstOrDefault'は、デフォルトのnull値を返すことができます。 Disguise.cs 192 Firstの代わりにFirstOrDefaultが



一般的に使用されるのはいつですか？選択が空の場合、FirstはInvalidOperationExceptionをスローします。FirstOrDefaultは例外をスローしませんが、参照タイプに対してnullを返します。 プロジェクトでは、ITooltipインターフェイスはさまざまなクラスによって実装されます。したがって、target.TraitsImplementing <ITooltip>（）が空の選択を返す場合、nullがtooltipに書き込まれます。



..。このオブジェクトのプロパティにさらにアクセスすると、NullReferenceExceptionが発生します。



開発者が選択範囲が空にならないことが確実な場合は、Firstを使用する方が適切です。よくわからない場合は、FirstOrDefaultによって返される値を確認する必要があります。これがここにないのはかなり奇妙です。結局のところ、前述のGetOrNullメソッドによって返される値は常にチェックされていました。なぜ彼らはここでそれをしなかったのですか？



誰が知っている...ああ、まさに！確かに、開発者はこれらの質問に答えることができます。最後に、彼はこのコードを編集する必要があります。

結論

OpenRAはどういうわけか、チェックするのが楽しくて面白いプロジェクトであることがわかりました。開発者は素晴らしい仕事をし、同時にソースが簡単に習得できるはずであることを忘れませんでした。もちろん、ここにはさまざまな...物議を醸すポイントがありますが、それらがない場合。



同時に、すべての努力をしても、開発者（alas）は人間のままです。考えられるポジティブのいくつかは、アナライザーを使用せずに気付くことは非常に困難です。書き込んだ直後でもエラーを見つけるのが難しい場合があります。久しぶりに見つけたとはどういうことか。



明らかに、その結​​果よりも間違いを検出する方がはるかに優れています。このために、膨大な数の新しいソースを手動で再チェックするのに何時間も費やすことができます。さて、古いものは同時に見えます-突然、以前に間違いに気づかなかったのですか？はい、レビューは本当に便利ですが、多くのコードを調べる必要がある場合は、時間の経過とともにいくつかのことに気付かなくなります。そして、それに多くの時間と労力が費やされています。





静的分析は、コードレビューなど、ソースコードの品質をチェックする他の方法への便利な追加です。 PVS-Studioは、開発者ではなく「単純な」（場合によってはそれだけではない）エラーを検出し、人々がより深刻な問題に集中できるようにします。



はい、アナライザーは誤検知を生成することがあり、すべてのエラーをまったく見つけることができません。しかし、それを使用すると、多くの時間と神経を節約できます。はい、彼は完璧ではなく、時には自分で間違いを犯します。ただし、一般的に、PVS-Studioを使用すると、開発プロセスがはるかに簡単になり、快適になり、さらに（予想外に！）安価になります。



実際、私の言葉を信じる必要はありません。上記の真実を自分で確認する方がはるかに良いでしょう。リンクをたどってアナライザーをダウンロードし、トライアルキーを入手してください。どれだけ簡単ですか？



まあ、それだけです。清聴ありがとうございました！コードと同じクリーンエラーログをクリーンアップしてください。

この記事を英語を話す聴衆と共有したい場合は、翻訳リンクを使用してください：NikitaLipilin。ユニコーンはRTSに侵入します：OpenRAソースコードの分析。