Единороги врываются в RTS: анализируем исходный код OpenRA

[hottabxp]

А почему разработчики таких крупных проектов не пользуются анализаторами кода?

[Krypt]

Последняя версия для C# — как-то она совсем ничего не ловит:

            if (centerY_ != null)
            {
                solver.AddConstraint(new ClLinearEquation(centerX, Cl.Divide(Cl.Plus(left, new ClLinearExpression(right)), new ClLinearExpression(2))));
            }
            if (centerY_ != null)
            {
                solver.AddConstraint(new ClLinearEquation(centerY, Cl.Divide(Cl.Plus(top, new ClLinearExpression(bottom)), new ClLinearExpression(2))));
            }

ошибку предлагаю найти с помощью вашего анализатора ;)

[Firensis]

Действительно, есть такая проблема. Дело в том, что у анализатор тут предполагает, что теоретически значение centerY_ может поменяться при вызове solver.AddConstraint. Однако человеческий глаз, конечно, легко видит тут странный момент, да. Мы подумаем над тем, как нам улучшить работу анализатора в подобных местах, спасибо за отзыв :)

[Krypt]

Собственно, он не видит эту ошибку, даже если я использую centerY вместо centerY_ в проверке. Но при этом правильно показывает ошибку, что centerY не может быть null:

        public ClVariable centerY
        {
            get
            {
                if (centerY_ == null)
                {
                    centerY_ = new ClVariable(NamePrefix() + "_cY");
                }
                return centerY_;
            }
        }

[Krypt]

Ещё больше не отловленных ошибок:

            ClEditInfo cei = _editVarMap[v];
            if (cei == null)
            {
                throw new CassowaryException("SuggestValue for variable " + v + ", but var is not an edit variable\n");
            }

Эта строго специфична для C#: _editVarMap — это System.Dictionary<TKey, TValue>
Dictionary выбрасывает KeyNotFountException если ключа нет в словаре

Правильным будет вот такой код:

            ClEditInfo cei = null;
            if (!_editVarMap.TryGetValue(v, out cei))
            {
                throw new CassowaryException("SuggestValue for variable " + v + ", but var is not an edit variable\n");
            }

Вы меня расстраиваете, вот честно.
Это из Cassowary.net, .Net порта резолвера систем линейных уравнений
Активный форк: github.com/martindevans/Cassowary.net
Мой форк: github.com/krypt-lynx/Cassowary.net

[Firensis]

ClEditInfo cei = _editVarMap[v];
if (cei == null)
{
    throw new CassowaryException("SuggestValue for variable " + v + ", but var is not an edit variable\n");
}

По этому фрагменту не соглашусь. В словарь вполне можно записать null. Например,

_editVarMap[v] = null;

[Krypt]

При том количестве ложно позитивных, что у вас есть, можно дать один полезный :P

В данном конкретном случае библиотека была портированна с Java, похоже там это таки валидная проверка. Вместо ожидаемой CassowaryException в соответствующих случаях библиотека кидает KeyNotFountException (и кривой юнит-тест, похоже, это пропускает)

Как я уже сказал, я несколько разочарован тем, что при наличии примитивных багов в библиотеке PVS-studio не показал ни одного. Но зато проругался на текст LGPL лиценции. (Эй, я только что вставил в заголовок «This is an open source non-commercial project.»!)

[Krypt]

Из того же проекта:

var builder = new StringBuilder('[')

У StringBuilder нет конструктора принимающего char. Так что параметр должен быть строкой:

var builder = new StringBuilder("[")

char же прозрачно кастуется к int. В результате код компилируется и вызывается конструктор

public StringBuilder(int capacity);

[Firensis]

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