Bobrovsky Oct 8 2018 at 19:24

Docotic.Pdf: Какие проблемы PVS-Studio обнаружит в зрелом проекте?

9 min

8.9K

.NET*C#*

+59

Comments 51

Andrey2008 Oct 8 2018 at 21:06

Спасибо за публикацию. Благодарим за критические замечания в вводной части статьи. Очень полезно получить взгляд на наш инструмент со стороны. Это будет нам поводом подумать, как сделать интерфейс лучше и понятнее. Также попробуем разобраться, что случилось со шрифтами.

P.S. Основное предназначение Standalone — это мониторинг запуска C и C++ компиляторов для сбора информации о строках запуска. Далее собранная информация используется для проверки файлов. В общем, это позволяет быстро проверить любой проект, не встраивая анализатор в систему сборки: www.viva64.com/ru/m/0031 Плюс Standalone можно использовать для просмотра отчётов.

Sabubu Oct 8 2018 at 21:51

> P.S. Основное предназначение Standalone — это мониторинг запуска C и C++ компиляторов для сбора информации о строках запуска.

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

CodeRush Oct 8 2018 at 21:32

c1 = -1;
… // Вот тут с с1 может и не случиться ничего, зависит от условий
freq[c1] += freq[c2];

Видимо не зря тут анализатор переживает…

apro Oct 8 2018 at 22:06

Там есть такие начальные условия

freq[256] = 1;
v = 1000000000L;

поэтому внутрь if поток управления должен попасть.

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

Imposeren Oct 8 2018 at 22:12

вроде бы из-за того, что freq[256] = 1, c1 как минимум будет равен 256, но конечно лучше реальные исходники посмотреть, непонятно что могли «упростить» в процессе обрезания кода для вставки в пост.

Bobrovsky Oct 9 2018 at 05:17

Реальные исходники этой части кода посмотреть можно.

paluke Oct 9 2018 at 06:49

А почему бы тогда сразу не проинициализировать с1 значением 256?

Imposeren Oct 9 2018 at 06:57

А вот это, как по мне, хорошее замечание. И тогда можно еще перед циклом назначать v = 1, вместо 1000000000L. Правда вот теперь я начинаю сомневаться и подозревать себя в невнимательности — уж очень странно получается…

Bobrovsky, что скажете: справедливы наши с paluke замечания?

P.S. а если предположить что элементы freq на самом деле не отрицательные, то c1 почти всегда будет 256. А если элементы вообще только положительные, то совсем всегда… И в итоге — просто всегда, т.к. нулевые значения игнорируются (if (freq[i] != 0 && freq[i] <= v)), т.е. первый цикл вообще бесполезен и можно сразу сделать c1=256. Так могут элементы freq быть отрицательными или нет? Если судить из «названия», то это частоты, и они отрицательными быть не должны.

P.P.S. Соответственно если значения не отрицательные, то и во втором цикле, при v == 1 можно делать break тем самым немного «оптимизировав код». Правда я больше питонист и не в курсе — может в C# break в каких-то странных случаях может ухудшать производительность, но думаю это маловероятно.

Imposeren Oct 9 2018 at 07:36

И всё-таки я был невнимателен: там еще есть внешний цикл for (;;), и поэтому freq[256] == 1 только первый раз, а второй раз это уже будет равно 1+freq[c2] из прошлого цикла. Но тогда и понятно от чего жалуется PVS: чисто формально, если добиться freq[c2] == 1000000000L, то на втором проходе внешнего бесконечного цикла уже возможна ситуация, когда нельзя будет попасть внутрь if, также подобная ситуация может возникнуть если в процессе работы цикла останутся только элементы равные 0, или больше 1000000000L. Так-что формально анализатор всё-таки прав. Плюс в примере в статье нужно добавить внешний бесконечный цикл и условия по которому он обрывается.

И считаю, что всё же стоит подумать: как сделать чтобы при первом проходе c1 определялось без цикла.

paluke Oct 9 2018 at 09:25

На самом деле, вместо 1000000000L там должно быть что-то типа Int64.MaxValue, не должно быть в массиве элементов больше, чем это значение.
Но это не мешает проинициализировать с1 значением 256. И эту инициализацию можно даже вынести из внешнего цикла, а сам цикл поиска значения с1 унести в самый конец этого самого внешнего «бесконечного» цикла. Вот тогда и получится определить с1 на первом проходе без цикла.

mayorovp Oct 10 2018 at 07:10

Не надо переносить поиск c1 в конец цикла! Это, может быть, и правда будет чуть быстрее и понятнее для роботов, но зато текущая версия понятнее для людей.

Всего-то нужно указать в комментариях характеристики цикла:

// invariant: sum of all items in freq array = const
// precondition: freq array contains as least one non zero item
// precondition: all items <= 256
// postcondition: freq array contains only one non zero item

После этого вопросов вида "может ли не найтись c1" возникать уже не должно...

Bobrovsky Oct 9 2018 at 18:25

Ваши замечания справедливы для случая произвольных значений в массиве freq. На практике, алгоритм работает с массивом не-отрицательных значений. Каждый элемент массива не больше 256.

vadlit Oct 9 2018 at 05:13

V3083 (возможный NRE при вызове евента) — попробуйте всегда использовать myevent?.Invoke(...) — о подобных проблемах не придётся думать.
Вопрос — были ли у вас дубли с предупреждениями решарпера?

Bobrovsky Oct 9 2018 at 05:14

Спасибо за совет. Дублей не было, потому что Решарпером мы не пользуемся.

Chosen0ne Oct 13 2018 at 08:39

или можно всегда ивенты объявлять так:

public event EventHandler myevent = delegate { };

и вызывать вообще без проверки на null.

Paull Oct 9 2018 at 07:03

Спасибо за статью!

Ложные срабатывания по V3081, V3134 обязательно посмотрим и скорее всего быстро поправим. По поводу же V3125, это известная проблема нашего C# анализатора сейчас — необходимо доработать механизмы dataflow и символьных вычислений, чтобы он смог понимать такие случаи. Здесь наш C# анализатор отстаёт от С/C++ анализатора, который это всё уже умеет. К сожалению, пока руки никак до этого не доходили, но надеемся, что до конца года (или в начале следующего) сможем и по этому направлению что-то сделать.

По поводу проверок возвращаемых значений методов, которые не могут вернуть null — нам уже отписывали подобные замечания\пожелания. Сейчас я склоняюсь к тому, чтобы во многом согласиться с вашими коллегами, которые агитировали за удаление этих избыточных проверок, тем более планируется расширить возможности анализатора диагностировать потенциальные null reference exception, и если контракт у таких методов когда-нибудь поменяется, статический анализатор также поможет вам обнаружить такие потенциальные исключения. Сейчас же я думаю, что мы просто понизим уровень подобных предупреждений, как некритичных.

neitri Oct 9 2018 at 08:16

Почему бы тогда не написать

freq[256] = 1;
// ....
c1 = 256;
// ....
freq[c1] += freq[c2];

Imposeren Oct 9 2018 at 09:24

Потому-что там еще есть внешний цикл, который в первой итерации поменяет freq[256] на 1+freq[c2]. Итого: из кода в статье «выкинули» важный кусок кода, и скорее всего анализатор всё-же прав

Upd: возможно я вас неправильно понял — я подумал что парвый цикл с поиском c1 вы совсем выкинули, но наверное подразумеваете, что он остаётся.

neitri Oct 10 2018 at 06:09

for (;;)
{
    /* Find the smallest nonzero frequency, set c1 = its symbol */
    /* In case of ties, take the larger symbol number */
    //c1 = -1;
    //v = 1000000000L;
	c1=256;
	for (i=0;i<=256;i++){
		if(freq[i]>0){
			c1=i;
			v=freq[i];
			break;
		}
	}
    for (i = c1+1; i <= 256; i++)
    {
	if (freq[i] != 0 && freq[i] <= v)
        {
            v = freq[i];
            c1 = i;
        }
    }

    /* Find the next smallest nonzero frequency, set c2 = its symbol */
    /* In case of ties, take the larger symbol number */
    //c2 = -1;
    //v = 1000000000L;
	c2=256;
	for (i=0;i<=256;i++){
		if(freq[i]>0){
                    c2=i;
                    v=freq[i];
                    break;
		}
	}
    for (i = c2; i <= 256; i++)
    {
        //if (freq[i] != 0 && freq[i] <= v && i != c1)
	if (freq[i] != 0 && freq[i] <= v && i)
        {
            v = freq[i];
            c2 = i;
        }
    }

    /* Done if we've merged everything into one frequency */
    //if (c2 < 0)
if (c1==c2)
    break;

	/* Else merge the two counts/trees */
    freq[c1] += freq[c2];
    freq[c2] = 0;

    /* Increment the codesize of everything in c1's tree branch */
    codesize[c1]++;
    while (others[c1] >= 0)
    {
        c1 = others[c1];
        codesize[c1]++;
    }

    others[c1] = c2;        /* chain c2 onto c1's tree branch */

    /* Increment the codesize of everything in c2's tree branch */
    codesize[c2]++;
    while (others[c2] >= 0)
    {
        c2 = others[c2];
        codesize[c2]++;
    }
}

mayorovp Oct 10 2018 at 06:17

Кажется, вы на пустом месте усложнили алгоритм.

А еще у вас всегда c1 будет равно c2, что является ошибкой. Там условие && i != c1 не просто так стояло...

paluke Oct 10 2018 at 06:44

Вот так:

freq[256] = 1;
c1 = 256;
for (;;)
{
    c2 = -1;
    v = 1000000000L;
    for (i = 0; i <= 256; i++)
    {
        if (freq[i] != 0 && freq[i] <= v && i != c1)
        {
            v = freq[i];
            c2 = i;
        }
    }

    /* Done if we've merged everything into one frequency */
    if (c2 < 0)
        break;

    /* Else merge the two counts/trees */
    freq[c1] += freq[c2];
    freq[c2] = 0;

    /* Increment the codesize of everything in c1's tree branch */
    codesize[c1]++;
    while (others[c1] >= 0)
    {
        c1 = others[c1];
        codesize[c1]++;
    }

    others[c1] = c2;        /* chain c2 onto c1's tree branch */

    /* Increment the codesize of everything in c2's tree branch */
    codesize[c2]++;
    while (others[c2] >= 0)
    {
        c2 = others[c2];
        codesize[c2]++;
    }

    v = 1000000000L;
    for (i = 0; i <= 256; i++)
    {
        if (freq[i] != 0 && freq[i] <= v)
        {
            v = freq[i];
            c1 = i;
        }
    }
}

mayorovp Oct 10 2018 at 07:07

Неужели этот код считается понятнее исходного?

paluke Oct 10 2018 at 07:29

Не, в нем на один проход по массиву меньше.

mayorovp Oct 10 2018 at 07:30

Экономия на спичках. Вот если бы переписать его через две очереди — это и правда было бы быстрее.

niq Oct 9 2018 at 09:07

В итоге, согласно принципу YAGNI, решили не держаться за проверки и удалили их. Все предупреждения были перенесены из теоретических/формальных в обоснованные.

У нас обычно такое правило — вызовам методов внутри одного класса доверяем и параметры не перепроверяем, а вот все приходящее снаружи — проверяем обязательно. Что-то типа зон доверия.