PVS-Studio corporate blog
.NET
Game development
Visual Studio
C#
Comments 8
+1
  if (!base.OnPressed(action))
    return false;

Ну такой межпроцедурный анализ — это, конечно, круто, но кажется, что вместо всего этого надо просто выдать один варнинг внутри updateResult:


return Judged; // value is always false

Ну можно на весь метод updateResult ругнуться, что он всегда возвращает false. Но протаскивать эту информацию на каждую точку вызова кажется довольно нелепо. Программирование — это про абстракцию и инкапсуляцию. Поведение updateResult инкапсулировано, по контракту (возвращаемый тип bool — это контракт) он может возвращать разное значение. Неочевидно, что стоит выдавать какие-то варнинги в точках вызова, это только шум. Точки вызова должны быть готовы к любой допустимой контрактом реализации метода.

+2
кажется, что вместо всего этого надо просто выдать один варнинг внутри updateResult

Анализатор внутри этого метода знает конечно, что оно всегда false, и если переменную с чем-то сравнивали бы, например, он бы на это ругнулся — но просто на return какого-то значения у нас диагностики нет.

А вот если бы этот return, возвращающий всегда false, зависел бы от входного значения параметра метода, передаваемого в изначальной точке вызова OnPressed? Тогда внутри самого метода мы бы не знали, что оно вернёт с таким входом false, и оставшийся кусок вызывающего метода станет недостижимым. А межпроцедурный анализ из точки вызова нам позволит это просчитать.

Неочевидно, что стоит выдавать какие-то варнинги в точках вызова, это только шум.

Инкапсуляция, исполнение контрактов — можно найти тысячу причин, почему что-то не стоит делать. А можно просто уметь это делать и находить сотни подобных интересных срабатываний, как это делает PVS-Studio )
+1
А можно просто уметь это делать и находить сотни подобных интересных срабатываний, как это делает PVS-Studio

Я согласен, что это интересное срабатывание для вас, и вы наверняка гордитесь таким межпроцедурным анализом, это всё понятно. Вопрос в том, есть ли от него польза пользователям. Где-то в другом месте может быть и есть, но здесь это кажется лишним шумом.

+5
Где-то в другом месте может быть и есть, но здесь это кажется лишним шумом.

Мне кажется, зачастую только автор кода может определить, является ли конкретное место ошибкой или нет, так что здесь я так категорически не стал бы судить.

Я согласен, что это интересное срабатывание для вас, и вы наверняка гордитесь таким межпроцедурным анализом, это всё понятно. Вопрос в том, есть ли от него польза пользователям.

Если же говорить про такой межпроцедурный анализ, то после того, как мы стали в прошлом году его расширять, например, у нашего C# анализатора стало больше фидбека от пользователей, больше интереса к нему и общения в поддержке. А несколько пользователей напрямую спрашивали именно о таких возможностях анализатора. Так что сейчас кажется, что мы сделали такой межпроцедурный анализ не зря.
0
public void UpdateProgress(double completionProgress)
{
  ....
  Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI);
  ....
}

Довольно очевидно, что серьёзной ошибки здесь нет. Во-первых, перепутанные x и y поменяют направление вращения. Если бы что-то вращалось не в ту сторону, то либо это бы сразу заметили, либо в принципе направление вращения здесь не важно (в играх такое вполне бывает). Я могу предположить, что автор действительно не разобрался, какие аргументы принимает Atan2, но потом добавил приседания с минусом и вычитанием 90 градусов, чтобы подогнать под требуемый результат. То есть формулу можно упростить, но понятно, что ошибки тут нет. Вообще когда речь идёт о вращениях на плоскости, x и y могут запросто переставиться. Такое эмпирическое предупреждение всегда шито белыми нитками.

+1

Замечу, что это код не самой игры, а с нуля переписанной новой версии, которая в будущем уже заменит оригинал

+1
Спасибо за статью, было интересно почитать.

При этом исходного кода не так много – 1813 файлов .cs, которые содержат 135 тысяч строк кода без учёта пустых. В этом коде присутствуют тесты, которые я обычно не учитываю в проверках. Код тестов содержится в 306 файлах .cs и, соответственно, 25 тысячах строк кода без учёта пустых. Это маленький проект: для сравнения, ядро C# анализатора PVS-Studio содержит около 300 тысяч строк кода.

Код который можно переиспользовать в других играх(обработка ввода, воспроизведение звуков и тд) вынесен в netstandard библиотеку. Т.е. на самом деле своего кода в проекте больше.
0
Да, наверное в статье следовало упомянуть про библиотеку osu-framework. Я проверял и её, но там нашлась всего одна интересная ошибка на 643 файла .cs и 66К непустых строк кода (без учета тестов). Для очистки совести — приведу эту ошибку :)

V3123 [CWE-783] Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. Drawable.cs 1187
public abstract partial class Drawable : Transformable, IDisposable, IDrawable
{
  ....
  public Vector2 AnchorPosition => 
    RelativeAnchorPosition * Parent?.ChildSize ?? Vector2.Zero;
  ....
}

Подобные ошибки были в статье. Не хватает скобок. Сейчас выражение вычисляется так:
 x = (a * b) ?? c

В коде osu-framework есть ещё порядка 10 хороших предупреждений «V3080 [CWE-476] Possible null dereference» про небезопасное использование потенциально нулевой ссылки, которая приходит из метода, но это уже скучновато.
Only those users with full accounts are able to leave comments.  , please.