Cet article est consacré à la vérification du projet OpenRA à l'aide de l'analyseur statique PVS-Studio. Qu'est-ce qu'OpenRA? C'est un moteur de jeu open source pour créer des jeux de stratégie en temps réel. L'article décrit comment l'analyse a été effectuée, quelles fonctionnalités du projet lui-même ont été découvertes et quels déclencheurs intéressants PVS-Studio a donné. Et, bien sûr, nous examinerons ici certaines des fonctionnalités de l'analyseur qui ont rendu le processus de vérification de projet plus confortable.
OpenRA
Le projet choisi pour examen est un moteur de jeu RTS dans le style de jeux comme Command & Conquer: Red Alert. Plus d'informations peuvent être trouvées sur le site Web . Le code source est écrit en C # et est disponible pour affichage et utilisation dans le référentiel .
OpenRA a été sélectionné pour examen pour 3 raisons. Premièrement, cela semble intéresser de nombreuses personnes. Dans tous les cas, cela s'applique aux habitants de GitHub, puisque le référentiel a collecté plus de 8 mille étoiles. Deuxièmement, la base de code OpenRA contient 1285 fichiers. Habituellement, ce montant est suffisant pour y trouver, espérons-le, des déclencheurs intéressants. Et troisièmement ... Les moteurs de jeu sont cool.
Extra positifs
J'ai analysé OpenRA à l'aide de PVS-Studio et j'ai été initialement inspiré par les résultats:
J'ai décidé que parmi tant d'avertissements élevés, je peux certainement trouver un tas de réponses différentes. Et, bien sûr, sur leur base, j'écrirai l'article le plus cool et le plus intéressant. Mais ce n'était pas là!
Il suffit de regarder les avertissements eux-mêmes et tout se met immédiatement en place. Sur les 1306 avertissements High, 1 277 étaient associés au diagnostic V3144 . Il affiche des messages comme "Ce fichier est marqué avec une licence copyleft, ce qui vous oblige à ouvrir le code source dérivé". Ce diagnostic est décrit plus en détail ici .
Evidemment, le fonctionnement d'un tel plan ne m'intéressait pas du tout, car OpenRA est déjà un projet open-source. Par conséquent, ils devaient être masqués pour ne pas interférer avec l'affichage du reste du journal. Comme j'utilisais un plugin pour Visual Studio, c'était facile à faire. Il vous suffit de faire un clic droit sur l'une des alertes V3144 et de sélectionner "Masquer toutes les erreurs V3144 " dans le menu qui s'ouvre .
Vous pouvez également choisir les avertissements qui seront affichés dans le journal en allant à la section "Erreurs détectables (C #)" dans les options de l'analyseur.
Pour y accéder en utilisant le plugin pour Visual Studio 2019, vous devez cliquer sur le menu supérieur Extensions-> PVS-Studio-> Options.
Résultats de test
Une fois les déclencheurs V3144 filtrés, il y a beaucoup moins d'avertissements dans le journal:
Néanmoins, nous avons réussi à trouver des moments intéressants parmi eux.
Conditions inutiles
De nombreux déclencheurs indiquaient des vérifications inutiles. Cela peut indiquer une erreur, car généralement les gens n'écrivent pas ce code exprès. Cependant, dans OpenRA, il semble que ces conditions inutiles aient été ajoutées volontairement. Par exemple:
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}Avertissement de l'analyseur : l' expression V3022 «active» est toujours vraie. SupportPowerManager.cs 206
PVS-Studio note à juste titre que la deuxième vérification n'a pas de sens, car si Active est faux , alors l'exécution ne l'atteindra pas. C'est peut-être une erreur, mais je pense que cela a été écrit exprès. Pourquoi? Eh bien pourquoi pas?
Peut-être avons-nous devant nous une sorte de solution temporaire, dont la révision est laissée «pour plus tard». Dans de tels cas, il est tout à fait pratique que l'analyseur rappelle au développeur de telles imperfections.
Regardons une autre vérification "juste au cas où":
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;
}
....
}Avertissement de l'analyseur : L' expression V3022 «highlightStart> 0» est toujours vraie. LabelWithHighlightWidget.cs 54
Encore une fois, il est clair qu'une nouvelle vérification n'a aucun sens. La valeur highlightStart est vérifiée deux fois dans les lignes adjacentes. Erreur? Il est possible que dans l'une des conditions, les mauvaises variables aient été sélectionnées pour le test. Quoi qu'il en soit, il est difficile de dire avec certitude de quoi il s'agit. Une chose est claire: le code doit être étudié et corrigé, ou une explication doit être laissée si une vérification supplémentaire est encore nécessaire pour une raison quelconque.
Voici un autre point similaire:
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;
}
....
}Avertissement de l'analyseur : V3063 Une partie de l'expression conditionnelle est toujours vraie si elle est évaluée: cancelButton! = Null. ConfirmationDialogs.cs 78
cancelButton peut en effet être nulle , parce que la valeur retournée par la GetOrNull méthode est écrit à cette variable . Cependant, il est logique de supposer que cancelButton dans le corps de l'opérateur conditionnel ne deviendra nullement nul . Néanmoins, il y a encore un chèque. Si vous ne faites pas attention à la condition externe, une situation étrange se produit en général: tout d'abord, les propriétés de la variable sont accessibles, puis le développeur a décidé de s'assurer - est-ce toujours nul ou non?
Au début, j'ai supposé que le projet pouvait utiliser une logique spécifique liée à la surcharge de l'opérateur "==". À mon avis, mettre en œuvre quelque chose de similaire pour les types de référence dans un projet est une idée controversée. Pourtant, le comportement inhabituel rend plus difficile pour les autres développeurs de comprendre le code. En même temps, il m'est difficile d'imaginer une situation où de telles astuces ne peuvent pas être supprimées. Bien qu'il soit probable que dans certains cas spécifiques, ce serait une solution pratique.
Dans le moteur de jeu Unity, par exemple, l'opérateur " == " est redéfini pour la classe UnityEngine.Object . La documentation officielle disponible sur le lien montre que la comparaison des instances de cette classe avec nullne fonctionne pas comme d'habitude. Eh bien, les développeurs avaient sûrement des raisons de mettre en œuvre une logique aussi inhabituelle.
Je n'ai pas trouvé quelque chose comme ça dans OpenRA :). Donc, s'il y a un sens dans les vérifications précédemment considérées pour null , alors cela consiste en autre chose.
PVS-Studio a pu détecter plusieurs autres moments similaires, mais il n'est pas nécessaire de tous les lister ici. C'est toujours ennuyeux de regarder les mêmes points positifs. Heureusement (ou pas), l'analyseur a également pu trouver d'autres bizarreries.
Code inaccessible
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}Avertissement de l'analyseur : l' expression V3022 '! Order.Queued && currentTransform.NextActivity! = Null' est toujours fausse. TransformsIntoTransforms.cs 44
Encore une fois, nous avons une vérification inutile. Certes, contrairement aux précédents, ici est présenté non seulement une condition supplémentaire, mais le code le plus réel inaccessible. Les vérifications précédemment considérées comme toujours vraies n'ont en fait pas d'incidence sur le fonctionnement du programme. Ils peuvent être supprimés du code ou laissés - rien ne changera.
Ici, une étrange vérification conduit au fait qu'une partie du code n'est pas exécutée. En même temps, il m’est difficile de deviner quels changements devraient être apportés ici sous forme d’amendement. Dans le cas le plus simple et le plus agréable, le code inaccessible ne doit tout simplement pas être exécuté. Alors il n'y a pas d'erreur. Cependant, je doute que le programmeur ait délibérément écrit la ligne juste pour la beauté.
Variable non initialisée dans le constructeur
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();
....
}
}Avertissement de l'analyseur : V3128 Le champ 'Frames' est utilisé avant d'être initialisé dans le constructeur. CursorSequence.cs 35
Un moment très désagréable. Tenter d'obtenir la valeur de la propriété Length à partir d'une variable non initialisée lèvera inévitablement une NullReferenceException . Dans une situation normale, une telle erreur ne passerait guère inaperçue - néanmoins, l'impossibilité de créer une instance d'une classe se révèle facilement. Mais ici l'exception ne sera levée que si la condition
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")sera vrai.
Il est difficile de juger de la manière dont vous devez corriger le code pour que tout fonctionne correctement. Je ne peux que supposer que la fonction devrait ressembler à ceci:
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();
....
}Dans cette version, le problème spécifié est absent, cependant, seul le développeur peut dire à quel point il correspond à l'idée originale.
Typo potentiel
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]);
....
}Avertissement de l'analyseur : V3127 Deux fragments de code similaires ont été trouvés. C'est peut-être une faute de frappe et la variable 'oldMapRamp' devrait être utilisée à la place de 'oldMapHeight' Map.cs 964
L'analyseur a détecté un moment suspect lié à la transmission d'arguments aux fonctions. Jetons un coup d'œil aux appels séparément:
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]);Curieusement, le dernier appel passe oldMapHeight , pas oldMapRamp . Bien sûr, tous ces cas ne sont pas vraiment des erreurs. Il est fort possible que tout soit écrit correctement ici. Mais vous devez admettre que cet endroit a l'air inhabituel. J'ai tendance à croire qu'une erreur a effectivement été commise ici.
Une note d'un collègue - Andrey Karpov . Et je ne vois rien d'étrange dans ce code. C'est une erreur classique de dernière ligne !
S'il n'y a toujours pas d'erreur ici, cela vaut la peine d'ajouter quelques explications. Après tout, si un moment ressemble à une erreur, quelqu'un voudra certainement le réparer.
Vrai, vrai et rien que vrai
Le projet contient des méthodes très particulières, dont la valeur de retour est de type booléen . Leur particularité réside dans le fait que dans toutes les conditions, ils retournent vrai . Par exemple:
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;
}Avertissement de l'analyseur : V3009 Il est étrange que cette méthode renvoie toujours une seule et même valeur de «true». LobbyCommands.cs 123
Ce code est-il correct? Y a-t-il une erreur? Cela semble très étrange. Pourquoi le développeur n'a-t-il pas utilisé void ?
Il n'est pas surprenant que l'analyseur considère un tel endroit comme étrange, mais nous devons tout de même admettre que le programmeur avait en fait une raison d'écrire de cette façon. Laquelle?
J'ai décidé de voir où cette méthode est appelée et si sa valeur de retour toujours vraie est utilisée. Il s'est avéré qu'il n'y avait qu'une seule référence dans la même classe - dans le dictionnaire commandHandlers , qui a le type
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>Lors de l'initialisation, des valeurs y sont ajoutées
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}etc.
On nous présente un cas rare (je veux le croire) où le typage statique nous pose des problèmes. Après tout, créer un dictionnaire dans lequel les valeurs seront des fonctions avec des signatures différentes ... est au moins problématique. commandHandlers n'est utilisé que dans la méthode InterpreterCommand :
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);
}Apparemment, l'objectif du développeur était la capacité universelle de faire correspondre les chaînes de certaines opérations effectuées. Je pense que la manière qu'il a choisie est loin d'être la seule, cependant, il n'est pas si facile de proposer quelque chose de plus pratique / correct dans une telle situation. Surtout si vous n'utilisez pas une sorte de dynamique ou quelque chose de similaire. Si vous avez des idées à ce sujet, laissez des commentaires. Il serait intéressant pour moi d'examiner diverses options pour résoudre ce problème.
Il s'avère que les avertissements associés aux méthodes toujours vraies de cette classe sont probablement faux. Et pourtant ... C'est ce "plus probable" qui vous fait peur. Vous devez être prudent avec de telles choses, car il peut bien y avoir une erreur parmi elles.
Tous ces positifs doivent être soigneusement vérifiés, puis marqués comme faux si nécessaire. Cela se fait tout simplement. Un commentaire spécial doit être laissé à l'endroit indiqué par l'analyseur:
static bool State(....) //-V3009Il existe un autre moyen: vous pouvez sélectionner les positifs qui doivent être marqués comme faux, et dans le menu contextuel, cliquez sur «Marquer les messages sélectionnés comme fausses alarmes».
Vous pouvez en savoir plus sur ce sujet dans la documentation .
Un chèque supplémentaire pour 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;
}Avertissement de l'analyseur : L' expression V3022 «lobbyInfo == null» est toujours fausse. LobbyCommands.cs 851
Une autre méthode qui renvoie toujours true . Cependant, cette fois, nous examinons un autre type de déclencheur. Vous devez étudier ces choses assez attentivement, car il est loin du fait qu'il ne s'agisse que d'un code redondant. Mais tout d'abord.
La méthode Deserialize ne retourne jamais null - vous pouvez facilement le vérifier en regardant son code:
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}J'ai raccourci le code source de la méthode pour la lisibilité. Vous pouvez le voir dans son intégralité en suivant le lien . Eh bien, ou croyez-moi, la variable de session ici ne se transforme en aucun cas en null .
Que voit-on en bas? Deserialize ne renvoie pas null , si quelque chose ne va pas, il lève des exceptions. Le développeur qui a écrit la vérification de null après l'appel semblait penser différemment. Très probablement, dans une situation exceptionnelle, la méthode SyncLobby devrait exécuter le code en cours d'exécution ... oui, elle n'est jamais exécutée, car lobbyInfo n'est jamais nul :
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}Je crois qu'au lieu de cette vérification "supplémentaire", vous devriez toujours utiliser try - catch . Eh bien, ou passez de l'autre côté et écrivez un TryDeserialize , qui en cas d'exception retournera null .
Possible 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; // <=
}
....
}Avertissement de l'analyseur : V3125 L'objet 'logo' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 236, 222. ConnectionLogic.cs 236
Quelque chose me dit qu'il y a une erreur de 100% ici. Nous n'avons certainement pas de vérifications «inutiles» devant nous, car la méthode GetOrNull peut très probablement retourner une référence nulle. Que se passe-t-il si le logo est nul ? L'appel de la propriété Bounds lèvera une exception, ce qui n'était clairement pas dans les plans du développeur.
Peut-être que le fragment doit être réécrit quelque chose comme ceci:
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;
}
}Cette option est assez simple à comprendre, bien que l'imbrication supplémentaire ne semble pas très cool. En tant que solution plus volumineuse, vous pouvez utiliser l'opérateur conditionnel nul:
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=Notez que j'aime mieux le correctif supérieur. Il est agréable de le lire et aucune question ne se pose. Mais certains développeurs attachent beaucoup d'importance à la brièveté, j'ai donc décidé de donner la deuxième option aussi :).
Peut-être que c'est OrDefault après tout?
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)
{
....
}
....
}Avertissement de l'analyseur : V3063 Une partie de l'expression conditionnelle est toujours vraie si elle est évaluée: terrainGeometryTrait! = Null. MapEditorLogic.cs 35
Analysons cet extrait de code . Notez que chaque fois que la méthode GetOrNull de la classe Widget est utilisée , elle est vérifiée pour null . Cependant, si Get est utilisé , il n'y a pas de validation. C'est logique - la méthode Get ne renvoie pas null :
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}Si l'élément n'est pas trouvé, une exception est levée - c'est un comportement raisonnable. Et en même temps, l'option logique serait de vérifier les valeurs retournées par la méthode GetOrNull pour l'égalité à une référence nulle.
Dans le code ci-dessus, la valeur renvoyée par la méthode Trait est vérifiée pour null . En fait, dans la méthode Trait , Get est appelé à partir de la classe TraitDictionary :
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}Se pourrait-il que ce Get se comporte différemment de celui discuté plus tôt? Pourtant, les classes sont différentes. Allons vérifier:
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}La méthode InnerGet renvoie une instance TraitContainer <T> . L'implémentation de Get dans cette classe est très similaire à Get dans la classe Widget :
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}La principale similitude est que null n'est jamais retourné ici non plus . En cas de problème, une InvalidOperationException est lancée de la même manière . Par conséquent, la méthode Trait se comporte de la même manière.
Oui, il peut y avoir juste un contrôle supplémentaire ici, qui n'affecte rien. Cela semble peut-être étrange, mais on ne peut pas dire qu'un tel code déroutera grandement le lecteur. Mais si la vérification est juste nécessaire ici, dans certains cas, une exception sera lancée de manière inattendue. C'est triste.
À ce stade, il semble plus approprié d'appeler une sorte de TraitOrNull . Cependant, une telle méthode n'existe pas :). Mais il y a TraitOrDefault , qui est analogue à GetOrNullpour ce cas.
Il existe un autre point similaire lié à la méthode Get :
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}Avertissement de l'analyseur : L' expression V3022 'frameSlider! = Null' est toujours vraie. AssetBrowserLogic.cs 128
Comme dans le code ci - dessus , quelque chose ne va pas ici. Soit la vérification est vraiment inutile, soit au lieu de Get , vous devez toujours appeler GetOrNull .
Mission perdue
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;
....
}Avertissement de l'analyseur : V3008 La variable 'widget.Bounds.Width' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 78, 75. SpawnSelectorTooltipLogic.cs 78
Il semble que si la condition textWidth! = CachedWidth est vraie , une valeur spécifique à la casse doit être écrite dans widget.Bounds.Width . Cependant, l'affectation ci-dessous, que cette condition soit vraie ou non, prive la chaîne
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;tous les sens. Il est probable qu'ils aient simplement oublié de mettre le reste ici :
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
);
}Vérification de la valeur par défaut
public void DisguiseAs(Actor target)
{
....
var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
AsPlayer = tooltip.Owner;
AsActor = target.Info;
AsTooltipInfo = tooltip.TooltipInfo;
....
}Avertissement de l'analyseur : V3146 Déréférence nulle possible de 'info-bulle'. Le 'FirstOrDefault' peut renvoyer une valeur nulle par défaut. Disguise.cs 192
Quand FirstOrDefault est- il couramment utilisé au lieu de First ? Si la sélection est vide, First lèvera une InvalidOperationException . FirstOrDefault ne lèvera pas d'exception, mais retournera null pour le type de référence.
Dans le projet, l'interface ITooltip est implémentée par différentes classes. Ainsi, si target.TraitsImplementing <ITooltip> () retourne une sélection vide, l' info - bulle sera écrite null... Un accès supplémentaire aux propriétés de cet objet entraînera une NullReferenceException .
Dans les cas où le développeur est sûr que la sélection ne sera pas vide, il est plus correct d'utiliser First . Si vous n'êtes pas si sûr, il vaut la peine de vérifier la valeur renvoyée par FirstOrDefault. Curieusement, ce n'est pas ici. Après tout, les valeurs renvoyées par la méthode GetOrNull évoquée précédemment étaient toujours vérifiées. Pourquoi ne l'ont-ils pas fait ici?
Qui sait ... Oh, exactement! Un développeur peut certainement répondre à ces questions. En fin de compte, il devrait éditer ce code.
Conclusion
OpenRA s'est en quelque sorte avéré être un projet agréable et intéressant à découvrir. Les développeurs ont fait un excellent travail et en même temps n'ont pas oublié que la source doit être facile à apprendre. Bien sûr, ici, il y a différents ... points controversés, mais où sans eux.
En même temps, malgré tous leurs efforts, les développeurs restent (hélas) humains. Certains des positifs considérés sont extrêmement difficiles à remarquer sans utiliser l'analyseur. Il est parfois difficile de trouver une erreur même immédiatement après l'avoir écrite. Que pouvons-nous dire de les trouver après une longue période.
Evidemment, il vaut bien mieux détecter une erreur que ses conséquences. Pour cela, vous pouvez passer des heures à revérifier manuellement un grand nombre de nouvelles sources. Eh bien, les anciens en même temps regardent - tout à coup n'a pas remarqué d'erreur plus tôt? Oui, les critiques sont vraiment utiles, mais si vous devez parcourir beaucoup de code, vous cessez de remarquer certaines choses avec le temps. Et beaucoup de temps et d'efforts y sont consacrés.
L'analyse statique n'est qu'un ajout pratique à d'autres méthodes de vérification de la qualité du code source, telles que la révision de code. PVS-Studio trouvera des erreurs «simples» (et parfois pas seulement) à la place du développeur, permettant aux gens de se concentrer sur des problèmes plus graves.
Oui, l'analyseur produit parfois de faux positifs et ne parvient pas du tout à trouver toutes les erreurs. Mais l'utiliser vous fera gagner beaucoup de temps et de nerfs. Oui, il n'est pas parfait et parfois il fait des erreurs lui-même. Cependant, en général, PVS-Studio rend le processus de développement beaucoup plus facile, plus agréable et même (de manière inattendue!) Moins cher.
En fait, vous n'avez pas besoin de me croire sur parole - il serait préférable de vérifier vous-même la véracité de ce qui précède. Suivez le lien pour télécharger l'analyseur et obtenir une clé d'essai. Combien plus facile?
Eh bien voilà tout. Merci de votre attention! Je vous souhaite un code propre et le même journal d'erreurs propre!
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Nikita Lipilin. Les licornes pénètrent dans RTS: analyse du code source OpenRA .