# Unicorns break into RTS: analyzing the OpenRA source code

This article is about the check of the OpenRA project using the static PVS\-Studio analyzer\. What is OpenRA? It is an open source game engine designed to create real\-time strategies\. The article describes the analysis process, project features, and warnings that PVS\-Studio has issued\. And, of course, here we will discuss some features of the analyzer that made the project checking process more comfortable\.

![0754_Checking_OpenRA/image1.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image1.png)

## OpenRA

![0754_Checking_OpenRA/image2.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image2.png)

The project chosen for the check is a game engine for RTS in the style of games such as Command & Conquer: Red Alert\. More information can be found on the [website](http://www.openra.net/)\. The source code is written in C\# and is available for viewing and using in the [repository](https://github.com/OpenRA/OpenRA)\.

There were 3 reasons for choosing OpenRA for a review\. First, it seems to be of interest to many people\. In any case, this applies to the inhabitants of GitHub, since the repository has reached the rating of more than 8 thousand stars\. Second, the OpenRA code base contains 1285 files\.  Usually this amount is quite enough to hope to find interesting warnings in them\. And third\.\.\. Game engines are cool :\)

## Redundant warnings

I analyzed OpenRA using PVS\-Studio and at first was encouraged by the results:

![0754_Checking_OpenRA/image3.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image3.png)

I decided that among so many High level warnings, I could definitely find a whole lot of different sapid errors\. Therefore, based on them, I would write the coolest and most intriguing article :\) But no such luck\!

One glance at the warnings and everything clicked into place\. 1,277 of the 1,306 High level warnings were related to the [V3144](https://pvs-studio.com/en/docs/warnings/v3144/) diagnostic\. It gives messages of the type "This file is marked with a copyleft license, which requires you to open the derived source code"\. This diagnostic is described in more detail [here](https://pvs-studio.com/en/docs/warnings/v3144/)\.

Obviously, I wasn't interested in warnings of such kind, as OpenRA is already an open source project\. Therefore, they had to be hidden so that they didn't interfere with viewing the rest of the log\. Since I used the Visual Studio plugin, it was easy to do so\. I just had to right\-click on one of the [V3144](https://pvs-studio.com/en/docs/warnings/v3144/) warnings and select "Hide all [V3144](https://pvs-studio.com/en/docs/warnings/v3144/) errors" in the opening menu\.

![0754_Checking_OpenRA/image5.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image5.png)

You can also choose which warnings will be displayed in the log by going to the "Detectable Errors \(C\#\)" section in the analyzer options\.

![0754_Checking_OpenRA/image7.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image7.png)

To go to them using the plugin for Visual Studio 2019, click on the top menu Extensions\-\>PVS\-Studio\-\>Options\.

## Check results

After the [V3144](https://pvs-studio.com/en/docs/warnings/v3144/) warnings were filtered out, there were significantly fewer warnings in the log:

![0754_Checking_OpenRA/image8.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image8.png)

Nevertheless, I managed to find worthy ones among them\.

### Meaningless conditions

Quite a few positives pointed to unnecessary checks\. This may indicate an error, because people usually don't write this code intentionally\. However, in OpenRA, it often looks as if these unnecessary conditions were added on purpose\. For example:

```cpp
public virtual void Tick()
{
  ....

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

  if (Active)
  {
    ....
  }
}
```

**Analyzer warning**:  [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'Active' is always true\. SupportPowerManager\.cs 206

PVS\-Studio quite rightly notes that the second check is meaningless, because if _Active_ is _false_, it won't execute\. It might be an error, but I think it was written intentionally\. What for? Well, why not?

Perhaps, what we have here is a temporary solution, which was supposed to be refined later\. In such cases, it is quite convenient that the analyzer will remind a developer of such shortcomings\.

Let's look at another just\-in\-case check:

```cpp
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;
  }
  ....
}
```

**Analyzer warning**: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'highlightStart \> 0' is always true\. LabelWithHighlightWidget\.cs 54

Again, it is obvious that re\-checking is completely pointless\. The value of _highlightStart_ is checked twice, right in neighbouring lines\. A mistake? It is possible that in one of the conditions wrong variables are selected for checking\. Anyway, it's hard to say for sure what's going on here\. One thing is definitely clear \- the code should be reviewed and corrected\. Or there should be an explanation if additional check is still needed for some reason\.

Here is another similar case:

```cpp
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;
  }
  ....
}
```

**Analyzer warning**: [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: cancelButton \!\= null\. ConfirmationDialogs\.cs 78

_cancelButton_ can be _null_ indeed, because the value returned by the _GetOrNull_ method is written to this variable\. However, it stands to reason that by no means will _cancelButton_ turn to _null_ in the body of the conditional operator\. Yet the check is still present\. If you don't pay attention to the external condition, you happen to be in a very strange situation\. First the variable properties are accessed, and then the developer decides to make sure if there is _null_ or not\.

At first, I assumed that the project might be using some specific logic related to overloading the "\=\=" operator\. In my opinion, implementing something like this in a project for reference types is a controversial idea\. Not to mention the fact that unusual behavior makes it harder for other developers to understand the code\. At the same time, it is difficult for me to imagine a situation where you can't do without such tricks\. Although it is likely that in some specific case this would be a convenient solution\.

In the Unity game engine, for example, the "_\=\=_" operator is redefined for the _UnityEngine\.Object_ class\. The official documentation available by the [link](https://docs.unity3d.com/ScriptReference/Object-operator_eq.html) shows that comparing instances of this _class_ with null doesn't work as usual\. Well, the developer probably had reasons for implementing this unusual logic\.

I didn't find anything like this in OpenRA :\)\. So if there is any meaning in the _null_ checks discussed earlier, it is something else\.

PVS\-Studio managed to find a few more similar cases, but there is no need to list them all here\. Well, it's a bit boring to watch the same triggers\. Fortunately \(or not\), the analyzer was able to find other oddities\.

### Unreachable code

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

  ....
}
```

**Analyzer warning**: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression '\!order\.Queued && currentTransform\.NextActivity \!\= null' is always false\. TransformsIntoTransforms\.cs 44

Once again, we have a pointless check here\. However, unlike the previous ones, this is not just an extra condition, but a real unreachable code\. The _always true_ checks above didn't actually affect the program's performance\. You can remove them from the code, or you can leave them – nothing will change\.

Whereas in this case, the strange check results in the fact that a part of the code isn't executed\.  At the same time, it is difficult for me to guess what changes should be made here as an amendment\. In the simplest and most preferable scenario, unreachable code simply shouldn't be executed\. Then there is no mistake\. However, I doubt that the programmer deliberately wrote the line just for the sake of beauty\.

### Uninitialized variable in the constructor

```cpp
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();

    ....
  }
}
```

**Analyzer warning**: [V3128](https://pvs-studio.com/en/docs/warnings/v3128/) The 'Frames' field is used before it is initialized in constructor\. CursorSequence\.cs 35

A nasty case\. An attempt to get the _Length_ property value from an uninitialized variable will inevitably result in the _NullReferenceException_\. In a normal situation, it is unlikely that such an error would have gone unnoticed – yet the inability to create an instance of the class is easily detected\. But here the exception will only be thrown if the condition

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

is true\. 

It is difficult to judge how to correct the code so that everything is fine\. I can only assume that the function should look something like this:

```cpp
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();

  ....
}
```

In this version, the stated problem is absent, but only the developer can tell to what extent it corresponds to the original idea\.

### Potential typo

```cpp
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]);  
  ....
}
```

**Analyzer warning**:  [V3127](https://pvs-studio.com/en/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'oldMapRamp' variable should be used instead of 'oldMapHeight' Map\.cs 964

The analyzer detected a suspicious fragment associated with passing arguments to the function\. Let's look at the calls separately:

```cpp
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]);
```

Oddly enough, the last call passes _oldMapHeight_, not _oldMapRamp_\. Of course, not all such cases are erroneous\. It is quite possible that everything is written correctly here\. But you will probably agree that this place looks unusual\. I'm inclined to believe that there is an error for sure\.

_Note by a colleague _[_Andrey Karpov_](https://pvs-studio.com/en/blog/posts/?author=andrey-karpov)_\. I don't see anything strange in this code :\)\. It's a classic [ last line mistake](https://pvs-studio.com/en/blog/posts/cpp/0260/)\!_

If there is no error, then one should add some explanation\. After all, if a snippet looks like an error, then someone will want to fix it\. 

### True, true and nothing but true

The project revealed very peculiar methods, the return value of which is of the _bool_ type\. Their uniqueness lies in the fact that they return _true_ under any conditions\. For example:

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

**Analyzer warning**: [V3009](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'true'\. LobbyCommands\.cs 123

Is everything OK in this code? Is there an error? It looks extremely strange\. Why haven't the developer used _void_?

It's not surprising that the analyzer finds such a place strange, but we still have to admit that the programmer actually had a reason to write this way\. Which one? 

I decided to check where this method is called and whether its returned _always true_ value is used\. It turned out that there is only one reference to it in the same class – in the _commandHandlers_ dictionary, which has the type

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

During the initialization, the following values are added to it

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

and others\.

Here we have a rare \(I'd like to think so\) case of static typing that creates problems for us\. After all, to make a dictionary in which the values are functions with different signatures\.\.\. is at least challenging\. _commandHandlers_ is only used in the _InterpretCommand_ method:

```cpp
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);
}
```

Apparently, the developer intended to have the universal possibility to match strings to certain operations\. I think that the chosen method is not the only one, but it is not so easy to offer something more convenient/correct in such a situation\. Especially if you don't use _dynamic_ or something like that\. If you have any ideas about this, please leave comments\. I would be interested to look at various solutions to this problem:\)\.

It turns out that warnings associated with _always true_ methods in this class are most likely false\. And yet\.\.\. What disquiets me here is this ''most likely'' :\) One has to really be careful and not miss an actual error among these positives\. 

All such warnings should be first carefully checked, and then marked as false if necessary\. You can simply do it\. You should leave a special comment in the place indicated by the analyzer:

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

There is another way: you can select the warnings that need to be marked as false, and click on "Mark selected messages as False Alarms" in the context menu\.

![0754_Checking_OpenRA/image10.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image10.png)

You can learn more about this topic in the [documentation](https://pvs-studio.com/en/docs/manual/0017/)\.

### Extra check for null?

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

**Analyzer warning**:  [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'lobbyInfo \=\= null' is always false\. LobbyCommands\.cs 851

Here we have another method that always returns _true_\. However, this time we are looking at a different type of a warning\. We have to pore over such places with all diligence, as there is no guarantee that we deal with redundant code\. But first things first\.

The _Deserialize_ method never returns _null_ – you can easily see this by looking at its code:

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

For ease of reading, I have shortened the source code of the method\. You can see it in full by clicking on the [link](https://github.com/OpenRA/OpenRA/blob/f642cead441446e16e565ac855b49186a899c253/OpenRA.Game/Network/Session.cs)\. Or take my word for it that the _session_ variable doesn't turn to _null_ under any circumstances\.

So what do we see at the bottom part? _Deserialize_ doesn't return _null_, and if something goes wrong, it throws exceptions\. The developer who wrote the _null_ check after the call was of a different mind, apparently\. Most likely, in an exceptional situation, the _SyncLobby_ method should execute the code that is currently being executed\.\.\. Actually, it is never executed, because _lobbyInfo_ is never _null_:

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

I believe that instead of this "extra" check, the author still needs to use _try_\-_catch_\. Or try another tack and write, let's say, _TryDeserialize_, which in case of an exceptional situation will return _null_\.

### Possible NullReferenceException

```cpp
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;                           // <=
  }
  ....
}
```

**Analyzer warning**: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'logo' object was used after it was verified against null\. Check lines: 236, 222\. ConnectionLogic\.cs 236

As for this case, I'm sure as hell there is an error\. We are definitely not looking at "extra" checks, because the _GetOrNull_ method can indeed return a null reference\. What happens if _logo_ is _null_? Accessing the _Bounds_ property will result in an exception, which was clearly not part of the developer's plans\.

Perhaps, the fragment needs to be rewritten in the following way:

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

This option is quite simple for comprehension, although the additional nesting doesn't look too great\. As a more comprehensive solution, one could use the null\-conditional operator:

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

By the way, the first version looks more preferable to me\. It is easy to read it and triggers no questions\. But some developers appreciate brevity quite highly, so I also decided to cite the second version as well :\)\.

### Maybe, OrDefault after all?

```cpp
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)
  {
    ....
  }

  ....
}
```

**Analyzer warning**: [V3063](https://pvs-studio.com/en/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: terrainGeometryTrait \!\= null\. MapEditorLogic\.cs 35

Let's delve into this fragment\. Note that each time the _GetOrNull_ method of the _Widget_ class is used, a _null_ equality check is performed\. However, if _Get_ is used, there is no check\. This is logical – the _Get_ method doesn't return _null_:

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

If the element is not found, an exception is thrown – this is reasonable behavior\. At the same time, the logical option would be to check the values returned by the _GetOrNull_ method for equality to the null reference\.

In the code above, the value returned by the _Trait_ method is checked for _null_\. Actually it is inside the _Trait_ method where _Get_ of the _TraitDictionary_ class is called:

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

Can it be that this _Get_ behaves differently from the one we discussed earlier? Well, the classes are different\. Let's check it out:

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

The _InnerGet_ method returns an instance of _TraitContainer<T\>_\. The _Get_ implementation in this class is very similar to _Get_ of the _Widget_ class:

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

The main similarity is that _null_ is never returned here either\. If something goes wrong, an _InvalidOperationException_ is similarly thrown\. Therefore, the _Trait_ method behaves the same way\.

Yes, there may just be an extra check that doesn't affect anything\. Except that it looks strange, but you can't say that this code will confuse a reader much\. But if the check is needed indeed, then in some cases an exception will be thrown unexpectedly\. It is sad\.

So in this fragment it seems more appropriate to call, for example, _TraitOrNull_\. However, there is no such method:\)\. But there is _TraitOrDefault_, which is the equivalent of _GetOrNull_ for this case\.

There is another similar case related to the _Get_ method:

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

**Analyzer warning**: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'frameSlider \!\= null' is always true\. AssetBrowserLogic\.cs 128

The same as in the code considered earlier, there is something wrong here\. Either the check is really unnecessary, or one still needs to call _GetOrNull_ instead of _Get_\.

### Lost assignment

```cpp
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;
  ....
}
```

**Analyzer warning** : [V3008](https://pvs-studio.com/en/docs/warnings/v3008/) The 'widget\.Bounds\.Width' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 78, 75\. SpawnSelectorTooltipLogic\.cs 78

It seems that if the _textWidth \!\= cachedWidth_ condition is true,  _widget\.Bounds\.Width_ must be written to a specific value for this case\. However, an assignment made below, regardless of whether this condition is true, makes the string

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

pointless\. It is likely that the author just forgot to write _else_ here:

```cpp
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
  );
}
```

### Checking the default value

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

**Analyzer warning**: [V3146](https://pvs-studio.com/en/docs/warnings/v3146/) Possible null dereference of 'tooltip'\. The 'FirstOrDefault' can return default null value\. Disguise\.cs 192

When is _FirstOrDefault_ usually used instead of _First_? If the selection is empty, _First_ throws an _InvalidOperationException_\. _FirstOrDefault_ doesn't throw an exception, but returns _null_ for the reference type\. 

The _ITooltip_ interface implements various classes in the project\. Thus, if _target\.TraitsImplementing<ITooltip\>\(\)_ returns an empty selection, _null_ is written to _tooltip_\. Accessing the properties of this object, which is executed next, will result in a _NullReferenceException_\.

In cases where the developer is sure that the selection won't be empty, it is better to use _First_\. If one isn't sure, it's worth checking the value returned by _FirstOrDefault\._ It is rather strange that we don't see it here\. After all, the values returned by the _GetOrNull_ method mentioned earlier were always checked\. Why didn't they do it here?

Who knows?\.\. All right, the developer will answer these questions for sure\. In the end, it is the code author who will be fixing it :\)

## Conclusion

OpenRA somehow turned out to be a project that was nice and interesting to scan\. The developers did a lot of work and didn't forget that the source code should be easy to view\. Of course, we did find some\.\.\. controversies, but one can't simply do without them :\)

At the same time, even with all the effort, alas, developers remain people\. Some of the considered warnings are extremely difficult to notice without using the analyzer\. It is sometimes difficult to find an error even immediately after writing it\. Needless to say, how hard it is to search for error after a long time\.

Obviously, it is much better to detect an error than its consequences\. To do this, you can spend hours rechecking a huge number of new sources manually\. Well, and have a bit of a look at the old ones \- what if there is an oversight there? Yes, reviews are really useful, but if you have to view a large amount of code, then you stop noticing some things over time\. And it takes a lot of time and effort\.

![0754_Checking_OpenRA/image11.png](https://import.viva64.com/docx/blog/0754_Checking_OpenRA/image11.png)

Static analysis is just a convenient addition to other methods of checking the quality of source code, such as code review\. PVS\-Studio will find "simple" \(and sometimes tricky\) errors instead of a developer, allowing people to focus on more serious issues\.

Yes, the analyzer sometimes gives false positives and is not able to find all the errors\. But with it you will save a lot of time and nerves\. Yes, it is not perfect and sometimes makes mistakes itself\. However, in general, PVS\-Studio makes the development process much easier, more enjoyable, and even \(unexpectedly\!\) cheaper :\)\.

In fact, you don't need to take my word for it \- it's much better to make sure that the above is true yourself\. You can use the [link](https://pvs-studio.com/en/pvs-studio/download/) to download the analyzer and get a trial key\. What could be simpler? :\)

Well, that's it for this time\. Thanks for your attention\! I wish you clean code and an empty error log\!