Andrey2008 5 фев 2021 в 12:49

Исследование COVID-19 и неинициализированная переменная

2 мин

3.9K

Блог компании PVS-StudioC++*Open source*Программирование*Совершенный код*

+15

Комментарии 25

datacompboy 5 фев 2021 в 12:55

Тесты… Тесты интересные, как я погляжу.

stranger777 5 фев 2021 в 13:07

double m = Data[row][col]; // numerator
double N = Data[row][col + 1]; // denominator

Нахватаю минусов, но умно не выглядит. Именование само по себе вынуждает пишущего такой код человека помнить, что такое m и N. А это уже потраченный ресурс, то есть потенциально причина ошибок по невнимательности, потому что внимание не резиновое. Проще было бы, будь именем то, что написано в комменте.

mobi 5 фев 2021 в 13:20

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

marsermd 5 фев 2021 в 13:58

Все правильно. Если есть стандартный нейминг из статьи, обычно его стоит соблюдать (особенно если в комментах приложена ссылка на описание алгоритма)

stranger777 5 фев 2021 в 21:38

Согласен, это работает, когда за поддержкой кода обращаешься к самому автору публикации. Да и то спорный вопрос: ведь и автору может быть проще читать код, который попросту понятнее, а не перевспоминать, что есть что. Это напрямую зависит от того, насколько понятно вообще написан код и насколько давно написана статья.

nickolaym 6 фев 2021 в 02:02

Если нейминг стандартный.
Я видел статью, написанную тремя соавторами, и там в разных абзацах были разные обозначения для одного и того же! И алгоритмические вставки — у одного в виде гигантской формулы с фигурной скобкой-ветвлением, у другого на псевдо-алголе, и третьего на псевдо, сука, коболе!
Да в гробу я видал тащить их обозначения в свой код.

ardraeiss 5 фев 2021 в 17:02

А потом в статье поедет шрифт. Или статья сама. А автора, который бы объяснил что за символы, уже нет — либо вообще, либо на конкретной работе. И попробуй перечитать…
Практический опыт, в старом документе к одной программулине с мат-расчётами(где все формулы были расписаны подробно) что-то стало со шрифтами(или был нестандартный, или формат в Ворде чуть поправили — уже не знаю и не узнаю) — и усё, половина формул потеряла читаемость, превратившись в "иероглифы".

Amomum 5 фев 2021 в 13:38

Интересно, чем (и с какими ключами) этот проект компилируется в норме? gcc вроде бы умеет о таком предупреждать.

Andrey2008 5 фев 2021 в 14:29

Это Visual C++, /W3. Предупреждения нет.

Все ключи

/JMC /permissive- /GS /W3 /Zc:wchar_t /I«R:\covid-sim-master\include» /ZI /Gm- /Od /sdl /Fd«x64\Debug\vc142.pdb» /Zc:inline /fp:precise /D «DO_OMP_PARALLEL» /D "_CRT_SECURE_NO_WARNINGS" /D "_DEBUG" /D "_CONSOLE" /D "_MBCS" /errorReport:prompt /WX- /Zc:forScope /RTC1 /Gd /MDd /openmp /std:c++14 /FC /Fa«x64\Debug\» /EHsc /nologo /Fo«x64\Debug\» /Fp«x64\Debug\CovidSim.pch» /diagnostics:column

Amomum 5 фев 2021 в 15:05

Хммм, но при этом есть Github Action, который собирает версию под линуксы; правда я что-то не могу сходу понять, с какими ключами. По идее нужна опция -Wuninitialized, которая часть -Wall.

crackedmind 5 фев 2021 в 16:44

но оно ж на /W4. Хотя оно и на /W1 есть 4700. Странно что не ругается

crackedmind 5 фев 2021 в 18:17

Все таки с /W4 хорошо отлавливает

src\CovidSim.cpp(5377) : warning C4701: potentially uninitialized local variable 'ModelValue' used

src\CovidSim.cpp(5412) : warning C4701: potentially uninitialized local variable 'ModelValue' used

Andrey2008 5 фев 2021 в 19:42

Теоретически, да :). На практике, /W4 даёт на проекте 828 предупреждения. В случае PVS-Studio, имеем 139 предупреждений общего назначения для всех трёх уровней. Уже легче. А если учесть, что 3 самых интересных сообщения живут на первом уровне достоверности, то становится понятно, почему ошибка ждала PVS-Studio, не смотря на то, что Visual C++ теоретически тоже может… ;)

datacompboy 5 фев 2021 в 20:23

/Wall это нечто что должно существовать с самого рождения.
добивать существующий проект — это буквально головой биться об эту самую стенку

crackedmind 5 фев 2021 в 21:31

Но как? Этот проект на /W4 выдал мне всего 138 предупреждений.

Понятно конечно, что PVS-Studio найдет еще более интересные проблемы :)

Andrey2008 5 фев 2021 в 21:55

Не так посмотрел и был не прав.

Это оказывается из-за IntelliSense вылезает. Однако, смотреть лог Visual C++ всё равно сложно. Например, он здесь ложно предупреждает о потенциально неинициализированной переменных SARI, Critical, CritRecov.

double SARI, Critical, CritRecov, incSARI, incCritical, incCritRecov, sc1, sc2double SARI, Critical, CritRecov, incSARI, incCritical, incCritRecov, sc1, sc2,sc3,sc4; //this stuff corrects bed prevalence for exponentially distributed time to test results in hospital
sc1 = (P.Mean_TimeToTest > 0) ? exp(-1.0 / P.Mean_TimeToTest) : 0.0;
sc2 = (P.Mean_TimeToTest > 0) ? exp(-P.Mean_TimeToTestOffset / P.Mean_TimeToTest) : 0.0;
sc3 = (P.Mean_TimeToTest > 0) ? exp(-P.Mean_TimeToTestCriticalOffset / P.Mean_TimeToTest) : 0.0;
sc4 = (P.Mean_TimeToTest > 0) ? exp(-P.Mean_TimeToTestCritRecovOffset / P.Mean_TimeToTest) : 0.0;
incSARI = incCritical = incCritRecov = 0;
for (i = 0; i < P.NumSamples; i++)
{
  if (i > 0)
  {
    SARI = (TSMean[i].SARI - TSMean[i - 1].SARI) * sc2 + SARI * sc1;   /// <<< C4701
    Critical = (TSMean[i].Critical - TSMean[i - 1].Critical) * sc3 + Critical * sc1; /// <<< C4701
    CritRecov = (TSMean[i].CritRecov - TSMean[i - 1].CritRecov) * sc4 + CritRecov * sc1; /// <<< C4701
    incSARI = TSMean[i].incSARI * (1.0 - sc2) + incSARI * sc1;
    incCritical = TSMean[i].incCritical * (1.0 - sc3) + incCritical * sc1;
    incCritRecov = TSMean[i].incCritRecov * (1.0 - sc4) + incCritRecov * sc1;
  }
  else
  {
    SARI = TSMean[i].SARI * sc2;
    Critical = TSMean[i].Critical * sc3;
    CritRecov = TSMean[i].CritRecov * sc4;
  }

Как следствие, быстро теряется желание всё это изучать.

crackedmind 5 фев 2021 в 22:07

Хм, а почему ложно?

double SARI;
// ...
SARI = (TSMean[i].SARI - TSMean[i - 1].SARI) * sc2 + SARI * sc1;

в конце выражения SARI * sc1 откуда возьмется значение?

ред.

а все, понял, в первый раз, мы попадем в else потом проинициализируем.

Написано конечно жуть)

slaFFik 5 фев 2021 в 14:32

Кто-то создал уже там issue c описанием, но даже не удосужился дочитать статью до конца :)
Вместо того, чтобы дать там ссылку на английский вариант статьи от авторов (https://habr.com/ru/company/pvs-studio/blog/541034/), Роман дал ссылку на гуглоперевод (https://translate.google.com/translate?sl=ru&tl=en&u=https://www.viva64.com/ru/b/0796/).

Krypt 5 фев 2021 в 16:07

форматирование этой формулы читабельности не помогает. Я в таких случаях стараюсь держать скобки в строке сбалансированными

Andrey2008 5 фев 2021 в 17:25

Кстати, там ещё вот такой забавный фрагмент есть:

void DigitalContactTracingSweep(double t)
{
  ....
  //check every day to see if contacts become index cases - but they have to be infectious. Otherwise we could set the trigger time and cause their contacts to be traced when they are not being traced themselves.
  if ((Hosts[contact].index_case_dct == 0) && (
    Hosts[contact].is_infectious_almost_symptomatic() ||
    Hosts[contact].is_case() ||
    Hosts[contact].is_infectious_almost_symptomatic()))
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions 'Hosts[contact].is_infectious_almost_symptomatic()' to the left and to the right of the '||' operator. Sweep.cpp 1354
Функция is_infectious_almost_symptomatic константная и ничего не изменяет:

bool Person::is_infectious_almost_symptomatic() const
{
  return this->inf == InfStat::InfectiousAlmostSymptomatic;
}

Так что два раза её звать точно смысла нет. Другое дело, что может это и не ошибка, а просто случайно продублированная проверка. А может и ошибка. По соседству вот такой код есть:

if (Hosts[contact].is_infectious_asymptomatic_not_case() ||
  Hosts[contact].is_case() ||
  Hosts[contact].is_infectious_almost_symptomatic())

Чтобы судить точнее, нужно, конечно, разбираться в проекте. Вот так со стороны непонятно.

crackedmind 5 фев 2021 в 21:40

А внутри первой проверки, есть еще одна

if ((Hosts[contact].index_case_dct == 0) && (
    Hosts[contact].is_infectious_almost_symptomatic() ||
    Hosts[contact].is_case() ||
    Hosts[contact].is_infectious_almost_symptomatic()))
{
    Hosts[contact].index_case_dct = 1;
    Hosts[contact].dct_trigger_time = ts + 1; 

    if (Hosts[contact].is_infectious_asymptomatic_not_case())
    {
        DoDetectedCase(contact, t, ts, tn);
        Hosts[contact].detected = 1; Hosts[contact].detected_time = ts;
    }
}

т.е. опечатка, иначе это условие не будет выполнятся

if (Hosts[contact].is_infectious_asymptomatic_not_case())

nickolaym 6 фев 2021 в 02:13

Хочется оторвать руки учоному, который написал эту формулу

LL += m * log((ModelValue + 1e-20) / (m / N + 1e-20)) +
        (N - m) * log((1.0 - ModelValue + 1e-20) / (1.0 - m / N + 1e-20));

Вот это 1e-20 означает "у меня тут где-то вылез логарифм нуля или деление на ноль, не хочу крешиться, не хочу проверять, хочу получить вырожденные значения с лютыми ошибками плавающей арифметики, авось прокатит".

v1000 6 фев 2021 в 14:34

Если такое делать, неужели нельзя было сделать переменную? Хотя лучше такое не делать.

Andrey2008 17 фев 2021 в 14:49

Небольшое продолжение: www.viva64.com/ru/b/0802

Andrey2008 20 мар 2021 в 18:12

Продолжение 2 :) — Пример, как в PVS-Studio появляются новые диагностики.

Зарегистрируйтесь на Хабре, чтобы оставить комментарий