Este artigo é dedicado a verificar o projeto OpenRA usando o analisador estático PVS-Studio. O que é OpenRA? É um motor de jogo de código aberto para a criação de jogos de estratégia em tempo real. O artigo descreve como a análise foi realizada, quais características do projeto em si foram descobertas e quais gatilhos interessantes o PVS-Studio deu. E, é claro, aqui consideraremos alguns dos recursos do analisador que tornaram o processo de verificação do projeto mais confortável.
OpenRA
O projeto selecionado para revisão é um motor de jogo RTS no estilo de jogos como Command & Conquer: Red Alert. Mais informações podem ser encontradas no site . O código-fonte é escrito em C # e está disponível para visualização e uso no repositório .
OpenRA foi selecionado para revisão por 3 razões. Em primeiro lugar, parece ser do interesse de muitas pessoas. Em todo caso, isso se aplica aos habitantes do GitHub, já que o repositório já coletou mais de 8 mil estrelas. Em segundo lugar, a base de código OpenRA contém 1285 arquivos. Normalmente, essa quantidade é suficiente para encontrar gatilhos interessantes neles. E em terceiro lugar ... Os motores de jogo são legais.
Positivos extras
Eu analisei OpenRA usando PVS-Studio e fui inicialmente inspirado pelos resultados:
Decidi que, entre tantos avisos altos, certamente posso encontrar muitas respostas muito diferentes. E, claro, com base neles, escreverei o artigo mais legal e interessante. Mas não estava lá!
Bastava olhar para os próprios avisos e tudo se encaixava imediatamente. Dos 1.306 avisos altos, 1.277 foram associados ao diagnóstico V3144 . Ele exibe mensagens como "Este arquivo está marcado com licença copyleft, que requer que você abra o código-fonte derivado". Este diagnóstico é descrito com mais detalhes aqui .
Obviamente, o funcionamento de tal plano não me interessou em nada, porque o OpenRA já é um projeto de código aberto. Portanto, eles precisavam ser ocultados para não interferir na visualização do restante do log. Como eu estava usando um plugin para Visual Studio, foi fácil de fazer. Você apenas tinha que clicar com o botão direito em um dos alertas V3144 e selecionar "Ocultar todos os erros V3144 " no menu que é aberto .
Você também pode escolher quais avisos serão exibidos no log, indo para a seção "Erros detectáveis (C #)" nas opções do analisador.
Para acessá-los usando o plugin para Visual Studio 2019, você precisa clicar no menu superior Extensões-> PVS-Studio-> Opções.
Resultado dos testes
Depois que os gatilhos V3144 foram filtrados, há significativamente menos avisos no log:
Mesmo assim, conseguimos encontrar momentos interessantes entre eles.
Condições inúteis
Muitos gatilhos indicaram verificações desnecessárias. Isso pode indicar um erro, porque geralmente as pessoas não escrevem esse código de propósito. No entanto, no OpenRA, muitas vezes parece que essas condições desnecessárias foram adicionadas propositalmente. Por exemplo:
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}Aviso do analisador : A expressão V3022 'Active' é sempre verdadeira. SupportPowerManager.cs 206
PVS-Studio corretamente observa que a segunda verificação não faz sentido, porque se Ativo for falso , a execução não o alcançará. Pode ser um erro, mas acho que foi escrito de propósito. Pelo que? Bem, porque não?
Talvez tenhamos diante de nós uma espécie de solução temporária, cuja revisão fica "para depois". Em tais casos, é bastante conveniente que o analisador lembre o desenvolvedor de tais imperfeições.
Vejamos mais uma verificação "por precauçã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;
}
....
}Aviso do analisador : V3022 A expressão 'highlightStart> 0' é sempre verdadeira. LabelWithHighlightWidget.cs 54
Novamente, está claro que a nova verificação é completamente sem sentido. O valor highlightStart é verificado duas vezes e em linhas adjacentes. Erro? É possível que em uma das condições as variáveis erradas tenham sido selecionadas para teste. De qualquer forma, é difícil dizer com certeza do que se trata. Uma coisa é clara - o código precisa ser estudado e corrigido ou uma explicação deve ser deixada se uma verificação adicional ainda for necessária por algum motivo.
Aqui está outro ponto semelhante:
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;
}
....
}Aviso do analisador : V3063 Uma parte da expressão condicional é sempre verdadeira se for avaliada: cancelButton! = Null. ConfirmationDialogs.cs 78
cancelButton pode realmente ser nulo , porque o valor retornado pelo método GetOrNull é gravado nesta variável . No entanto, é lógico supor que cancelButton no corpo do operador condicional não se tornará nulo de forma alguma . No entanto, ainda há uma verificação. Se você não prestar atenção à condição externa, em geral uma situação estranha acontece: primeiro, as propriedades da variável são acessadas e, em seguida, o desenvolvedor decidiu ter certeza - ainda é nula ou não?
A princípio, presumi que o projeto poderia usar alguma lógica específica relacionada à sobrecarga do operador "==". Na minha opinião, implementar algo semelhante para tipos de referência em um projeto é uma ideia controversa. Ainda assim, o comportamento incomum torna mais difícil para outros desenvolvedores entender o código. Ao mesmo tempo, é difícil para mim imaginar uma situação em que tais truques não possam ser dispensados. Embora seja provável que, em algum caso específico, esta seja uma solução conveniente.
No mecanismo de jogo Unity, por exemplo, o operador " == " é redefinido para a classe UnityEngine.Object . A documentação oficial disponível nos links mostra que comparando instâncias desta classe com nulonão funciona como de costume. Bem, certamente os desenvolvedores tinham motivos para implementar essa lógica incomum.
Não encontrei algo assim no OpenRA :). Portanto, se houver algum sentido nas verificações de nulo consideradas anteriormente , isso consiste em outra coisa.
O PVS-Studio conseguiu encontrar vários outros momentos semelhantes, mas não há necessidade de listá-los todos aqui. Ainda é chato assistir aos mesmos pontos positivos. Felizmente (ou não), o analisador também conseguiu encontrar outras esquisitices.
Código inacessível
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}Aviso do analisador : V3022 Expression '! Order.Queued && currentTransform.NextActivity! = Null' é sempre falso. TransformsIntoTransforms.cs 44
Novamente, temos uma verificação inútil. É verdade que, em contraste com os anteriores, aqui é apresentada não apenas uma condição extra, mas o código mais real inatingível. As verificações antes consideradas sempre verdadeiras de fato não afetaram o funcionamento do programa. Eles podem ser removidos do código ou podem ser deixados - nada mudará.
Aqui, uma verificação estranha leva ao fato de que parte do código não é executada. Ao mesmo tempo, é difícil para mim adivinhar quais mudanças devem ser feitas aqui como uma emenda. No caso mais simples e agradável, o código inalcançável simplesmente não deve ser executado. Então não há engano. No entanto, duvido que o programador tenha escrito a linha deliberadamente apenas por questão de beleza.
Variável não inicializada no construtor
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();
....
}
}Aviso do analisador : V3128 O campo 'Quadros' é usado antes de ser inicializado no construtor. CursorSequence.cs 35
Um momento muito desagradável. Uma tentativa de obter o valor da propriedade Length de uma variável não inicializada resultará inevitavelmente em um NullReferenceException sendo lançado . Em uma situação normal, tal erro dificilmente passaria despercebido - no entanto, a impossibilidade de criar uma instância de uma classe se revela facilmente. Mas aqui a exceção só será lançada se a condição
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")será verdade.
É difícil julgar como você precisa corrigir o código para fazer tudo funcionar bem. Só posso supor que a função deve ser semelhante a esta:
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();
....
}Nesta versão, o problema especificado está ausente, entretanto, apenas o desenvolvedor pode dizer o quanto ele corresponde à ideia original.
Possível erro de digitação
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]);
....
}Aviso do analisador : V3127 Dois fragmentos de código semelhantes foram encontrados. Talvez seja um erro de digitação e a variável 'oldMapRamp' deva ser usada em vez de 'oldMapHeight' Map.cs 964
O analisador detectou um momento suspeito relacionado à passagem de argumentos para funções. Vamos dar uma olhada nas chamadas separadamente:
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]);Curiosamente, a última chamada passa oldMapHeight , não oldMapRamp . Claro, nem todos esses casos são realmente erros. É bem possível que tudo esteja escrito corretamente aqui. Mas você deve admitir que este lugar parece incomum. Estou inclinado a acreditar que um erro realmente foi cometido aqui.
Uma nota de um colega - Andrey Karpov . E não vejo nada de estranho neste código. Este é o clássico erro de última linha !
Se ainda não houver erro aqui, então vale a pena acrescentar alguma explicação. Afinal, se um momento parece um erro, alguém definitivamente vai querer consertá-lo.
Verdadeiro, verdadeiro e nada além de verdade
O projeto contém alguns métodos muito peculiares, cujo valor de retorno é do tipo bool . Sua peculiaridade reside no fato de que sob quaisquer condições eles retornam verdadeiros . Por exemplo:
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;
}Aviso do analisador : V3009 É estranho que esse método sempre retorne um e o mesmo valor de 'verdadeiro'. LobbyCommands.cs 123
Este código está correto? Existe algum erro? Parece muito estranho. Por que o desenvolvedor não usou void ?
Não é surpreendente que o analisador considere tal lugar estranho, mas ainda temos que admitir que o programador na verdade tinha uma razão para escrever dessa forma. Qual?
Decidi ver onde esse método é chamado e se seu valor de retorno sempre verdadeiro está sendo usado. Descobriu-se que há apenas uma referência a ele na mesma classe - no dicionário commandHandlers , que tem o tipo
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>Durante a inicialização, os valores são adicionados a ele
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}etc.
Somos apresentados a um caso raro (quero acreditar) em que a tipagem estática cria problemas para nós. Afinal, fazer um dicionário no qual os valores serão funções com assinaturas diferentes ... é no mínimo problemático. commandHandlers é usado apenas no método 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);
}Aparentemente, o objetivo do desenvolvedor era a capacidade universal de combinar sequências de certas operações realizadas. Acho que o caminho que ele escolheu está longe de ser o único, porém, não é tão fácil oferecer algo mais conveniente / correto em tal situação. Principalmente se você não usar algum tipo de dinâmica ou algo semelhante. Se você tem alguma ideia sobre este assunto, deixe comentários. Seria interessante para mim examinar várias opções para resolver esse problema.
Acontece que os avisos associados aos métodos sempre verdadeiros nesta classe são provavelmente falsos. E ainda ... É esse "mais provável" que te assusta.Você precisa ter cuidado com essas coisas, porque entre elas pode sim haver um engano.
Todos esses positivos devem ser verificados cuidadosamente e, em seguida, marcados como falsos, se necessário. Isso é feito de forma bastante simples. Um comentário especial deve ser deixado no local indicado pelo analisador:
static bool State(....) //-V3009Existe outra forma: você pode selecionar os positivos que precisam ser marcados como falsos e, no menu de contexto, clicar em "Marcar mensagens selecionadas como alarmes falsos".
Você pode aprender mais sobre este tópico na documentação .
Verificação extra para nulo?
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;
}Aviso do analisador : A expressão V3022 'lobbyInfo == null' é sempre falsa. LobbyCommands.cs 851
Outro método que sempre retorna verdadeiro . No entanto, desta vez, estamos examinando um tipo diferente de gatilho. Você precisa estudar essas coisas com bastante cuidado, porque está longe de ser o fato de que se trata apenas de código redundante. Mas as primeiras coisas primeiro.
O método Deserialize nunca retorna nulo - você pode verificar isso facilmente observando seu código:
public static Session Deserialize(string data)
{
try
{
var session = new Session();
....
return session;
}
catch (YamlException)
{
throw new YamlException(....);
}
catch (InvalidOperationException)
{
throw new YamlException(....);
}
}Para facilitar a leitura, encurtei o código-fonte do método. Você pode ver na íntegra clicando no link . Bem, ou acredite que a variável de sessão aqui em nenhuma circunstância se torna nula .
O que vemos no fundo? Deserializar não retorna nulo , se algo deu errado, ele lança exceções. O desenvolvedor que escreveu o cheque para null após a chamada parecia pensar de forma diferente. Muito provavelmente, em uma situação excepcional, o método SyncLobby deve executar o código que está sendo executado ... sim, ele nunca é executado, porque lobbyInfo nunca é nulo :
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}Acredito que em vez dessa verificação "extra", você ainda deve usar try - catch . Bem, ou vá do outro lado e escreva um TryDeserialize , que retornará null no caso de uma exceção .
Possível 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; // <=
}
....
}Aviso do analisador : V3125 O objeto 'logo' foi usado após ser verificado em relação ao nulo. Verifique as linhas: 236, 222. ConnectionLogic.cs 236
Algo me diz que há um erro de 100% aqui. Definitivamente, não temos verificações "extras" diante de nós, porque o método GetOrNull, muito provavelmente, pode realmente retornar uma referência nula. O que acontece se o logotipo for nulo ? Chamar a propriedade Bounds lançará uma exceção, que claramente não estava nos planos do desenvolvedor.
Talvez o fragmento precise ser reescrito mais ou menos assim:
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;
}
}Essa opção é simples de entender, embora o aninhamento adicional não pareça muito legal. Como uma solução mais ampla, você pode usar o operador nulo-condicional:
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=Observe que eu gosto mais da correção superior. É agradável lê-lo e não há dúvidas. Mas alguns desenvolvedores valorizam muito a brevidade, então decidi dar a segunda opção também :).
Talvez seja OrDefault, afinal?
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)
{
....
}
....
}Aviso do analisador : V3063 Uma parte da expressão condicional é sempre verdadeira se for avaliada: terrainGeometryTrait! = Null. MapEditorLogic.cs 35
Vamos analisar este snippet. Observe que cada vez que o método GetOrNull da classe Widget é usado , é verificado se há nulo . No entanto, se Get for usado , não haverá validação. Isso é lógico - o método Get não retorna nulo :
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}Se o elemento não for encontrado, uma exceção é lançada - esse é um comportamento razoável. E, ao mesmo tempo, a opção lógica seria verificar os valores retornados pelo método GetOrNull quanto à igualdade com uma referência nula.
No código acima, o valor retornado pelo método Trait é verificado para nulo . Na verdade, dentro do método Trait , Get é chamado da classe TraitDictionary :
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}Será que este Get se comporta de maneira diferente do discutido anteriormente? No entanto, as classes são diferentes. Vamos checar:
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}O método InnerGet retorna uma instância de TraitContainer <T> . A implementação de Get nesta classe é muito semelhante a Get na classe Widget :
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}A principal semelhança é que null também nunca é retornado aqui . Caso algo dê errado, uma InvalidOperationException é lançada da mesma forma . Portanto, o método Trait se comporta da mesma maneira.
Sim, pode haver apenas uma verificação extra aqui, o que não afeta nada. Pode parecer estranho, mas não se pode dizer que tal código confunda muito o leitor. Mas se a verificação for necessária apenas aqui, em alguns casos, uma exceção será lançada inesperadamente. É triste.
Neste ponto, parece mais apropriado chamar algum tipo de TraitOrNull . No entanto, esse método não existe :). Mas há TraitOrDefault , que é análogo a GetOrNullpara este caso.
Há outro ponto semelhante relacionado ao método Get :
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}Aviso do analisador : A expressão V3022 'frameSlider! = Null' é sempre verdadeira. AssetBrowserLogic.cs 128
Como no código acima , algo está errado aqui. Ou a verificação é realmente desnecessária ou, em vez de Get , você ainda precisa chamar GetOrNull .
Tarefa perdida
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;
....
}Aviso do analisador : V3008 A variável 'widget.Bounds.Width' recebe valores duas vezes sucessivamente. Talvez seja um engano. Verifique as linhas: 78, 75. SpawnSelectorTooltipLogic.cs 78
Parece que se a condição textWidth! = CachedWidth for verdadeira , algum valor específico deve ser escrito em widget.Bounds.Width . No entanto, a atribuição abaixo, independentemente de esta condição ser verdadeira, priva a string
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;todos os sentidos. É provável que eles simplesmente tenham se esquecido de colocar o else aqui :
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
);
}Verificando o valor padrão
public void DisguiseAs(Actor target)
{
....
var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
AsPlayer = tooltip.Owner;
AsActor = target.Info;
AsTooltipInfo = tooltip.TooltipInfo;
....
}Aviso do analisador : V3146 Possível desreferência nula de 'dica de ferramenta'. O 'FirstOrDefault' pode retornar um valor nulo padrão. Disguise.cs 192
Quando FirstOrDefault é comumente usado em vez de First ? Se a seleção estiver vazia, o First lançará uma InvalidOperationException . FirstOrDefault não lançará uma exceção, mas retornará null para o tipo de referência.
No projeto, a interface ITooltip é implementada por várias classes. Assim, se target.TraitsImplementing <ITooltip> () retornar uma seleção vazia, null será escrito na dica de ferramenta... O acesso posterior às propriedades deste objeto resultará em uma NullReferenceException .
Nos casos em que o desenvolvedor tem certeza de que a seleção não ficará vazia, é mais correto usar Primeiro . Se você não tiver tanta certeza, vale a pena verificar o valor retornado por FirstOrDefault. Curiosamente, isso não está aqui. Afinal, os valores retornados pelo método GetOrNull discutido anteriormente sempre foram verificados. Por que eles não fizeram isso aqui?
Quem sabe ... Oh, exatamente! Certamente um desenvolvedor pode responder a essas perguntas. No final, ele deve editar esse código.
Conclusão
O OpenRA acabou se tornando um projeto agradável e interessante de verificar. Os desenvolvedores fizeram um ótimo trabalho e ao mesmo tempo não se esqueceram que o código-fonte deve ser fácil de aprender. Claro, aqui existem diferentes ... pontos controversos, mas onde sem eles.
Ao mesmo tempo, mesmo com todos os seus esforços, os desenvolvedores (infelizmente) permanecem humanos. Alguns dos positivos considerados são extremamente difíceis de perceber sem o uso do analisador. Às vezes, é difícil encontrar um erro, mesmo imediatamente após escrevê-lo. O que podemos dizer sobre encontrá-los depois de muito tempo.
Obviamente, é muito melhor detectar um erro do que suas consequências. Para isso, você pode passar horas verificando manualmente um grande número de novas fontes. Bem, os antigos, ao mesmo tempo, olham - de repente, não percebeu nenhum erro antes? Sim, as revisões são realmente úteis, mas se você tiver que examinar muitos códigos, com o tempo você deixará de notar algumas coisas. E muito tempo e esforço são gastos nisso.
A análise estática é apenas uma adição conveniente a outros métodos de verificação da qualidade do código-fonte, como revisão de código. O PVS-Studio encontrará erros "simples" (e às vezes não apenas) no lugar do desenvolvedor, permitindo que as pessoas se concentrem em questões mais sérias.
Sim, o analisador às vezes produz falsos positivos e não consegue encontrar todos os erros. Mas usá-lo vai economizar muito tempo e nervos. Sim, ele não é perfeito e às vezes ele mesmo comete erros. Porém, em geral, o PVS-Studio torna o processo de desenvolvimento muito mais fácil, mais agradável e até (inesperadamente!) Mais barato.
Na verdade, você não precisa acreditar apenas na minha palavra - seria muito melhor verificar você mesmo a verdade do que foi dito acima. Siga o link para baixar o analisador e obter uma chave de teste. Muito mais fácil?
Bom, isso é tudo. Obrigado pela atenção! Desejo a você um código limpo e o mesmo log de erro limpo!
Se você deseja compartilhar este artigo com um público de língua inglesa, por favor, use o link de tradução: Nikita Lipilin. Unicórnios invadem RTS: analisando o código-fonte OpenRA .