Comments 8
if (!base.OnPressed(action))
return false;Ну такой межпроцедурный анализ — это, конечно, круто, но кажется, что вместо всего этого надо просто выдать один варнинг внутри updateResult:
return Judged; // value is always falseНу можно на весь метод updateResult ругнуться, что он всегда возвращает false. Но протаскивать эту информацию на каждую точку вызова кажется довольно нелепо. Программирование — это про абстракцию и инкапсуляцию. Поведение updateResult инкапсулировано, по контракту (возвращаемый тип bool — это контракт) он может возвращать разное значение. Неочевидно, что стоит выдавать какие-то варнинги в точках вызова, это только шум. Точки вызова должны быть готовы к любой допустимой контрактом реализации метода.
кажется, что вместо всего этого надо просто выдать один варнинг внутри updateResult
Анализатор внутри этого метода знает конечно, что оно всегда false, и если переменную с чем-то сравнивали бы, например, он бы на это ругнулся — но просто на return какого-то значения у нас диагностики нет.
А вот если бы этот return, возвращающий всегда false, зависел бы от входного значения параметра метода, передаваемого в изначальной точке вызова OnPressed? Тогда внутри самого метода мы бы не знали, что оно вернёт с таким входом false, и оставшийся кусок вызывающего метода станет недостижимым. А межпроцедурный анализ из точки вызова нам позволит это просчитать.
Неочевидно, что стоит выдавать какие-то варнинги в точках вызова, это только шум.
Инкапсуляция, исполнение контрактов — можно найти тысячу причин, почему что-то не стоит делать. А можно просто уметь это делать и находить сотни подобных интересных срабатываний, как это делает PVS-Studio )
А можно просто уметь это делать и находить сотни подобных интересных срабатываний, как это делает PVS-Studio
Я согласен, что это интересное срабатывание для вас, и вы наверняка гордитесь таким межпроцедурным анализом, это всё понятно. Вопрос в том, есть ли от него польза пользователям. Где-то в другом месте может быть и есть, но здесь это кажется лишним шумом.
Где-то в другом месте может быть и есть, но здесь это кажется лишним шумом.
Мне кажется, зачастую только автор кода может определить, является ли конкретное место ошибкой или нет, так что здесь я так категорически не стал бы судить.
Я согласен, что это интересное срабатывание для вас, и вы наверняка гордитесь таким межпроцедурным анализом, это всё понятно. Вопрос в том, есть ли от него польза пользователям.
Если же говорить про такой межпроцедурный анализ, то после того, как мы стали в прошлом году его расширять, например, у нашего C# анализатора стало больше фидбека от пользователей, больше интереса к нему и общения в поддержке. А несколько пользователей напрямую спрашивали именно о таких возможностях анализатора. Так что сейчас кажется, что мы сделали такой межпроцедурный анализ не зря.
public void UpdateProgress(double completionProgress)
{
....
Rotation = -90 + (float)(-Math.Atan2(diff.X, diff.Y) * 180 / Math.PI);
....
}Довольно очевидно, что серьёзной ошибки здесь нет. Во-первых, перепутанные x и y поменяют направление вращения. Если бы что-то вращалось не в ту сторону, то либо это бы сразу заметили, либо в принципе направление вращения здесь не важно (в играх такое вполне бывает). Я могу предположить, что автор действительно не разобрался, какие аргументы принимает Atan2, но потом добавил приседания с минусом и вычитанием 90 градусов, чтобы подогнать под требуемый результат. То есть формулу можно упростить, но понятно, что ошибки тут нет. Вообще когда речь идёт о вращениях на плоскости, x и y могут запросто переставиться. Такое эмпирическое предупреждение всегда шито белыми нитками.
Замечу, что это код не самой игры, а с нуля переписанной новой версии, которая в будущем уже заменит оригинал
При этом исходного кода не так много – 1813 файлов .cs, которые содержат 135 тысяч строк кода без учёта пустых. В этом коде присутствуют тесты, которые я обычно не учитываю в проверках. Код тестов содержится в 306 файлах .cs и, соответственно, 25 тысячах строк кода без учёта пустых. Это маленький проект: для сравнения, ядро C# анализатора PVS-Studio содержит около 300 тысяч строк кода.
Код который можно переиспользовать в других играх(обработка ввода, воспроизведение звуков и тд) вынесен в netstandard библиотеку. Т.е. на самом деле своего кода в проекте больше.
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» про небезопасное использование потенциально нулевой ссылки, которая приходит из метода, но это уже скучновато.
В «osu!» играй, про ошибки не забывай