Este artículo está dedicado a verificar el proyecto OpenRA utilizando el analizador estático PVS-Studio. ¿Qué es OpenRA? Es un motor de juego de código abierto para crear juegos de estrategia en tiempo real. El artículo describe cómo se llevó a cabo el análisis, qué características del proyecto en sí se descubrieron y qué disparadores interesantes dio PVS-Studio. Y, por supuesto, aquí consideraremos algunas de las características del analizador que hicieron más cómodo el proceso de verificación del proyecto.
OpenRA
El proyecto elegido para revisión es un motor de juego RTS al estilo de juegos como Command & Conquer: Red Alert. Puede encontrar más información en el sitio web . El código fuente está escrito en C # y está disponible para su visualización y uso en el repositorio .
OpenRA fue seleccionado para revisión por 3 razones. Primero, parece ser de interés para muchas personas. En cualquier caso, esto se aplica a los habitantes de GitHub, ya que el repositorio ha recolectado más de 8 mil estrellas. En segundo lugar, el código base de OpenRA contiene 1285 archivos. Por lo general, esta cantidad es suficiente para encontrar desencadenantes interesantes en ellos. Y en tercer lugar ... Los motores de juego son geniales.
Positivos extra
Analicé OpenRA usando PVS-Studio y los resultados me inspiraron inicialmente:
Decidí que entre tantas advertencias altas, ciertamente puedo encontrar muchas respuestas muy diferentes. Y, por supuesto, sobre esa base escribiré el artículo más genial e interesante. ¡Pero no estaba allí!
Uno solo tenía que mirar las advertencias en sí, y todo encajó de inmediato. De las 1306 advertencias altas, 1277 se asociaron con el diagnóstico V3144 . Muestra mensajes como "Este archivo está marcado con una licencia copyleft, que requiere que abra el código fuente derivado". Este diagnóstico se describe con más detalle aquí .
Obviamente, no estaba interesado en poner en marcha tal plan, porque OpenRA ya es un proyecto de código abierto. Por lo tanto, debían estar ocultos para que no interfirieran con la visualización del resto del registro. Como estaba usando un complemento para Visual Studio, fue fácil de hacer. Solo tenía que hacer clic derecho en una de las alertas de V3144 y seleccionar "Ocultar todos los errores de V3144 " en el menú que se abre .
También puede elegir qué advertencias se mostrarán en el registro yendo a la sección "Errores detectables (C #)" en las opciones del analizador.
Para ir a ellos usando el complemento para Visual Studio 2019, debe hacer clic en el menú superior Extensiones-> PVS-Studio-> Opciones.
Resultados de la prueba
Después de filtrar los desencadenantes de V3144 , hay significativamente menos advertencias en el registro:
Sin embargo, logramos encontrar momentos interesantes entre ellos.
Condiciones sin sentido
Varios desencadenantes indicaron controles innecesarios. Esto puede indicar un error, porque generalmente la gente no escribe dicho código a propósito. Sin embargo, en OpenRA, a menudo parece que estas condiciones innecesarias se agregaron a propósito. Por ejemplo:
public virtual void Tick()
{
....
Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
if (!Active)
return;
if (Active)
{
....
}
}Advertencia del analizador : V3022 Expression 'Active' siempre es verdadera. SupportPowerManager.cs 206
PVS-Studio señala con bastante razón que la segunda verificación no tiene sentido, porque si Active es falso , la ejecución no lo alcanzará. Puede ser un error aquí, pero creo que fue escrito a propósito. ¿Para qué? ¿Bueno, por qué no?
Quizás tengamos ante nosotros una especie de solución temporal, cuya revisión se deja "para más tarde". En tales casos, es muy conveniente que el analizador le recuerde al desarrollador tales imperfecciones.
Veamos una comprobación más "por si acaso":
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;
}
....
}Advertencia del analizador : la expresión V3022 'highlightStart> 0' siempre es verdadera. LabelWithHighlightWidget.cs 54
Una vez más, está claro que volver a comprobar no tiene ningún sentido. El valor highlightStart se comprueba dos veces y en líneas adyacentes. ¿Error? Es posible que en una de las condiciones se seleccionaron las variables incorrectas para la prueba. De cualquier manera, es difícil decir con certeza de qué se trata. Una cosa está clara: el código debe estudiarse y corregirse, o debe dejarse una explicación si aún se necesita una verificación adicional por alguna razón.
Aquí hay otro punto similar:
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;
}
....
}Advertencia del analizador : V3063 Una parte de la expresión condicional siempre es verdadera si se evalúa: cancelButton! = Null. ConfirmationDialogs.cs 78
cancelButton puede ser nulo , porque el valor devuelto por el método GetOrNull se escribe en esta variable . Sin embargo, es lógico considerar que en el cuerpo de la sentencia condicional cancelButton de ninguna manera se convierte en nulo . Sin embargo, todavía hay un cheque. Si no presta atención a la condición externa, en general, resulta una situación extraña: primero, se accede a las propiedades de la variable y luego el desarrollador decidió asegurarse: ¿sigue siendo nula o no?
Al principio, asumí que el proyecto podría estar usando alguna lógica específica relacionada con la sobrecarga del operador "==". En mi opinión, implementar algo similar para los tipos de referencia en un proyecto es una idea controvertida. Aún así, el comportamiento inusual dificulta que otros desarrolladores comprendan el código. Al mismo tiempo, me resulta difícil imaginar una situación en la que no se pueda prescindir de tales trucos. Aunque es probable que en algún caso concreto, esta sea una solución conveniente.
En el motor de juego de Unity, por ejemplo, el operador " == " se redefine para la clase UnityEngine.Object . La documentación oficial disponible en el enlace muestra que comparar instancias de esta clase con nullno funciona como de costumbre. Bueno, seguramente los desarrolladores tenían razones para implementar una lógica tan inusual.
No encontré algo así en OpenRA :). Entonces, si hay algún sentido en las comprobaciones consideradas anteriormente para nulo , entonces consiste en algo más.
PVS-Studio pudo encontrar varios momentos similares más, pero no es necesario enumerarlos todos aquí. Sigue siendo aburrido ver los mismos aspectos positivos. Afortunadamente (o no), el analizador también pudo encontrar otras rarezas.
Código inalcanzable
void IResolveOrder.ResolveOrder(Actor self, Order order)
{
....
if (!order.Queued || currentTransform == null)
return;
if (!order.Queued && currentTransform.NextActivity != null)
currentTransform.NextActivity.Cancel(self);
....
}Advertencia del analizador : la expresión V3022 '! Order.Queued && currentTransform.NextActivity! = Null' siempre es falsa. TransformsIntoTransforms.cs 44
De nuevo tenemos una comprobación sin sentido. Sin embargo, a diferencia de los anteriores, aquí se presenta no solo una condición adicional, sino un código realmente inalcanzable. Las comprobaciones previamente consideradas siempre verdaderas de hecho no afectaron el funcionamiento del programa. Se pueden eliminar del código o se pueden dejar; nada cambiará.
Aquí, una comprobación extraña lleva al hecho de que parte del código no se ejecuta. Al mismo tiempo, me resulta difícil adivinar qué cambios deberían introducirse aquí como enmienda. En el caso más simple y agradable, el código inalcanzable simplemente no debería ejecutarse. Entonces no hay error. Sin embargo, dudo que el programador haya escrito deliberadamente la línea solo por la belleza.
Variable no inicializada en el constructor
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();
....
}
}Advertencia del analizador : V3128 El campo 'Frames' se usa antes de que se inicialice en el constructor. CursorSequence.cs 35
Un momento muy desagradable. Un intento de obtener el valor de la propiedad Length de una variable no inicializada resultará inevitablemente en la generación de una NullReferenceException . En una situación normal, tal error difícilmente pasaría desapercibido; sin embargo, la imposibilidad de crear una instancia de una clase se revela fácilmente. Pero aquí la excepción solo se lanzará si la condición
(d.ContainsKey("Length") && d["Length"].Value == "*") ||
(d.ContainsKey("End") && d["End"].Value == "*")será verdad.
Es difícil juzgar cómo necesitas arreglar el código para que todo funcione bien. Solo puedo asumir que la función debería verse así:
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();
....
}En esta versión, el problema especificado está ausente, sin embargo, solo el desarrollador puede decir cuánto corresponde a la idea original.
Posible error tipográfico
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]);
....
}Advertencia del analizador : V3127 Se encontraron dos fragmentos de código similares. Quizás este sea un error tipográfico y se debería usar la variable 'oldMapRamp' en lugar de 'oldMapHeight' Map.cs 964
El analizador ha detectado un momento sospechoso relacionado con el paso de argumentos a funciones. Echemos un vistazo a las llamadas por separado:
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, la última llamada pasa a oldMapHeight , no a oldMapRamp . Por supuesto, no todos estos casos son realmente errores. Es muy posible que todo esté escrito correctamente aquí. Pero debes admitir que este lugar parece inusual. Me inclino a creer que aquí se ha cometido un error.
Una nota de un colega, Andrey Karpov . Y no veo nada extraño en este código. ¡Este es el clásico error de última línea !
Si todavía no hay ningún error aquí, entonces vale la pena agregar alguna explicación. Después de todo, si un momento parece un error, definitivamente alguien querrá arreglarlo.
Verdadero, cierto y nada más que cierto
El proyecto contiene métodos muy peculiares, cuyo valor de retorno tiene el tipo bool . Su peculiaridad radica en el hecho de que bajo cualquier condición vuelven verdaderos . Por ejemplo:
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;
}Advertencia del analizador : V3009 Es extraño que este método siempre devuelva el mismo valor de "verdadero". LobbyCommands.cs 123
¿Está bien este código? ¿Hay algún error? Parece muy extraño. ¿Por qué el desarrollador no usó void ?
No es de extrañar que el analizador considere extraño un lugar así, pero aun así tenemos que admitir que el programador realmente tenía una razón para escribir de esta manera. ¿Cúal?
Decidí ver dónde se llama a este método y si se está utilizando su valor de retorno siempre verdadero . Resultó que solo hay una referencia a él en la misma clase: en el diccionario commandHandlers , que tiene el tipo
IDictionary<string, Func<S, Connection, Session.Client, string, bool>>Durante la inicialización, se le agregan valores
{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}etc.
Se nos presenta un caso raro (quiero creerlo) en el que la escritura estática nos crea problemas. Después de todo, hacer un diccionario en el que los valores serán funciones con diferentes firmas ... es al menos problemático. commandHandlers solo se usa en el 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, el objetivo del desarrollador era la capacidad universal de hacer coincidir cadenas de ciertas operaciones realizadas. Creo que la forma que ha elegido está lejos de ser la única, sin embargo, no es tan fácil ofrecer algo más conveniente / correcto en tal situación. Especialmente si no usas algún tipo de dinámica o algo similar. Si tiene alguna idea sobre este tema, deje comentarios. Sería interesante para mí ver varias opciones para resolver este problema.
Resulta que las advertencias asociadas con los métodos siempre verdaderos en esta clase son probablemente falsas. Y sin embargo ... es este "muy probable" lo que te asusta, debes tener cuidado con esas cosas, porque entre ellas puede que haya un error.
Todos estos positivos deben revisarse cuidadosamente y luego marcarse como falsos si es necesario. Esto se hace de forma muy sencilla. Se debe dejar un comentario especial en el lugar indicado por el analizador:
static bool State(....) //-V3009Hay otra forma: puede seleccionar los positivos que deben marcarse como falsos y, en el menú contextual, hacer clic en "Marcar mensajes seleccionados como falsas alarmas".
Puede obtener más información sobre este tema en la documentación .
¿Comprobación adicional por 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;
}Advertencia del analizador : La expresión V3022 'lobbyInfo == null' siempre es falsa. LobbyCommands.cs 851
Otro método que siempre devuelve verdadero . Sin embargo, esta vez estamos examinando un tipo diferente de desencadenante. Necesita estudiar estas cosas con suficiente cuidado, porque está lejos del hecho de que esto sea solo un código redundante. Pero lo primero es lo primero.
El método Deserialize nunca devuelve un valor nulo ; puede verificar esto fácilmente mirando su 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(....);
}
}He acortado el código fuente del método para facilitar la lectura. Puedes verlo completo siguiendo el enlace . Bueno, o créame que la variable de sesión aquí bajo ninguna circunstancia se convierte en nula .
¿Qué vemos en la parte inferior? Deserialize no devuelve nulo , si algo salió mal arroja excepciones. El desarrollador que emitió el cheque por nulo después de la llamada pareció pensar de manera diferente. Lo más probable es que, en una situación excepcional, el método SyncLobby deba ejecutar el código que se está ejecutando actualmente ... sí, nunca se ejecuta, porque lobbyInfo nunca es nulo :
if (lobbyInfo == null)
{
server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
return true;
}Creo que en lugar de esta comprobación "adicional", debería utilizar try - catch . Bueno, o ve desde el otro lado y escribe algo de TryDeserialize , que devolverá nulo en caso de una excepción .
Posible 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; // <=
}
....
}Advertencia del analizador : V3125 El objeto 'logo' se utilizó después de que se verificara con un valor nulo. Verifique las líneas: 236, 222. ConnectionLogic.cs 236
Algo me dice que hay un error del 100% aquí. Definitivamente no tenemos comprobaciones "adicionales" ante nosotros, porque el método GetOrNull, muy probablemente, realmente puede devolver una referencia nula. ¿Qué pasa si el logo es nulo ? Llamar a la propiedad Bounds resultará en el lanzamiento de una excepción, que claramente no estaba en los planes del desarrollador.
Quizás el fragmento deba reescribirse de esta manera:
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;
}
}Esta opción es bastante simple de entender, aunque el anidamiento adicional no se ve muy bien. Como solución más amplia, puede utilizar el operador condicional nulo:
// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=Tenga en cuenta que me gusta más la solución superior. Es agradable leerlo y no surgen preguntas. Pero algunos desarrolladores valoran mucho la brevedad, así que decidí dar la segunda opción también :).
¿Quizás sea OrDefault después de todo?
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)
{
....
}
....
}Advertencia del analizador : V3063 Una parte de la expresión condicional siempre es verdadera si se evalúa: terrainGeometryTrait! = Null. MapEditorLogic.cs 35
Analicemos este fragmento. Tenga en cuenta que cada vez que se utiliza el método GetOrNull de la clase Widget , se comprueba si es nulo . Sin embargo, si se usa Get , no hay validación. Esto es lógico: el método Get no devuelve nulo :
public T Get<T>(string id) where T : Widget
{
var t = GetOrNull<T>(id);
if (t == null)
throw new InvalidOperationException(....);
return t;
}Si no se encuentra el elemento, se lanza una excepción: este es un comportamiento razonable. Y al mismo tiempo, la opción lógica sería verificar que los valores devueltos por el método GetOrNull sean iguales a una referencia nula.
En el código anterior, el valor devuelto por el método Trait se verifica como nulo . De hecho, dentro del método Trait , se llama a Get desde la clase TraitDictionary :
public T Trait<T>()
{
return World.TraitDict.Get<T>(this);
}¿Podría ser que este Get se comporte de manera diferente al discutido anteriormente? Sin embargo, las clases son diferentes. Vamos a revisar:
public T Get<T>(Actor actor)
{
CheckDestroyed(actor);
return InnerGet<T>().Get(actor);
}El método InnerGet devuelve una instancia de TraitContainer <T> . La implementación de Get en esta clase es muy similar a la de Get en la clase Widget :
public T Get(Actor actor)
{
var result = GetOrDefault(actor);
if (result == null)
throw new InvalidOperationException(....);
return result;
}La principal similitud es que aquí tampoco se devuelve nulo . En caso de que algo saliera mal, se lanza una InvalidOperationException de manera similar . Por lo tanto, el método Trait se comporta de la misma manera.
Sí, puede que haya un cheque adicional aquí, que no afecta nada. Quizás parezca extraño, pero no se puede decir que tal código confundirá mucho al lector. Pero si la verificación solo es necesaria aquí, en algunos casos se lanzará una excepción inesperadamente. Es triste.
En este punto, parece más apropiado llamar a algún tipo de TraitOrNull . Sin embargo, no existe tal método :). Pero hay TraitOrDefault , que es análogo a GetOrNullpara este caso.
Hay otro punto similar relacionado con el método Get :
public AssetBrowserLogic(....)
{
....
frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
if (frameSlider != null)
{
....
}
....
}Advertencia del analizador : la expresión V3022 'frameSlider! = Null' siempre es verdadera. AssetBrowserLogic.cs 128
Como en el código anterior , algo está mal aquí. O la verificación es realmente innecesaria o, en lugar de Get , aún debe llamar a GetOrNull .
Tarea 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;
....
}Advertencia del analizador : V3008 A la variable 'widget.Bounds.Width' se le asignan valores dos veces seguidas. Quizás esto sea un error. Verifique las líneas: 78, 75. SpawnSelectorTooltipLogic.cs 78
Parece que si la condición textWidth! = CachedWidth es verdadera , se debe escribir algún valor específico del caso en widget.Bounds.Width . Sin embargo, la asignación a continuación, independientemente de si esta condición es verdadera, priva a la cadena
widget.Bounds.Width = 2 * label.Bounds.X + textWidth;todos los sentidos. Es probable que simplemente se hayan olvidado de poner el else aquí :
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
);
}Comprobando el valor predeterminado
public void DisguiseAs(Actor target)
{
....
var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
AsPlayer = tooltip.Owner;
AsActor = target.Info;
AsTooltipInfo = tooltip.TooltipInfo;
....
}Advertencia del analizador : V3146 Posible desreferencia nula de 'información sobre herramientas'. El 'FirstOrDefault' puede devolver un valor nulo predeterminado. Disguise.cs 192 ¿
Cuándo se usa comúnmente FirstOrDefault en lugar de First ? Si la selección está vacía, First lanzará una InvalidOperationException . FirstOrDefault no generará una excepción, pero devolverá nulo para el tipo de referencia.
En el proyecto, la interfaz ITooltip es implementada por varias clases. Por lo tanto, si target.TraitsImplementing <ITooltip> () devuelve una selección vacía, se escribirá nulo en la información sobre herramientas... Un mayor acceso a las propiedades de este objeto resultará en una NullReferenceException .
En los casos en que el desarrollador esté seguro de que la selección no estará vacía, es más correcto usar First . Si no está tan seguro, vale la pena comprobar el valor devuelto por FirstOrDefault. Curiosamente, esto no está aquí. Después de todo, los valores devueltos por el método GetOrNull discutido anteriormente siempre se verificaron. ¿Por qué no lo hicieron aquí?
Quién sabe ... ¡Oh, exactamente! Seguramente un desarrollador puede responder estas preguntas. Al final, debería editar este código.
Conclusión
De alguna manera, OpenRA resultó ser un proyecto agradable e interesante de ver. Los desarrolladores han hecho un gran trabajo y al mismo tiempo no han olvidado que la fuente debe ser fácil de aprender. Por supuesto, aquí hay diferentes ... puntos controvertidos, pero sin ellos.
Al mismo tiempo, incluso con todos sus esfuerzos, los desarrolladores (por desgracia) siguen siendo humanos. Algunos de los positivos considerados son extremadamente difíciles de notar sin usar el analizador. A veces es difícil encontrar un error incluso inmediatamente después de escribirlo. ¿Qué podemos decir sobre encontrarlos después de mucho tiempo?
Evidentemente, es mucho mejor detectar un error que sus consecuencias. Para ello, puede pasar horas revisando manualmente una gran cantidad de fuentes nuevas. Bueno, los viejos al mismo tiempo miran - ¿de repente no notaron ningún error antes? Sí, las revisiones son realmente útiles, pero si tienes que revisar mucho código, con el tiempo dejas de notar algunas cosas. Y se dedica mucho tiempo y esfuerzo a ello.
El análisis estático es solo una adición conveniente a otros métodos para verificar la calidad del código fuente, como la revisión de código. PVS-Studio encontrará errores "simples" (y a veces no solo) en lugar del desarrollador, lo que permitirá a las personas concentrarse en problemas más serios.
Sí, el analizador a veces produce falsos positivos y no puede encontrar todos los errores. Pero usarlo te ahorrará mucho tiempo y nervios. Sí, no es perfecto y, a veces, él mismo comete errores. Sin embargo, en general, PVS-Studio hace que el proceso de desarrollo sea mucho más fácil, más agradable e incluso (¡inesperadamente!) Más económico.
De hecho, no es necesario que confíe en mi palabra; sería mucho mejor verificar usted mismo la verdad de lo anterior. Siga el enlace para descargar el analizador y obtener una clave de prueba. ¿Cuánto más fácil?
Bueno eso es todo. ¡Gracias por su atención! ¡Le deseo un código limpio y el mismo registro de errores limpio!
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Nikita Lipilin. Los unicornios irrumpen en RTS: analizando el código fuente de OpenRA .