Я хочу посвятить эту статью проблеме, о которой мало кто задумывается. Все шире и шире применяется моделирование различных процессов с помощью компьютера. Это замечательно, возможностью экономить время и материалы на бессмысленные химические, биологические, физические и прочие эксперименты. Обдув модели крыла на компьютере может в несколько раз сократить количество макетов, которое затем будет испытываться в реальной аэродинамической трубе. Численным экспериментам доверяют всё больше. Однако за торжеством численного моделирования никто не обращает внимания на рост сложности программ. В компьютере и к программам видят всего лишь инструмент для получения результата. Меня тревожит, что далеко не все знают и задумываются о том, что рост размера программы ведет к нелинейному росту числа ошибок. Опасно применять компьютер как просто большой калькулятор. Я и думаю, надо доносить эту мысль до других людей.
Большой калькулятор
В начале, я хотел назвать эту статью приблизительно так «Если программистам нельзя изготавливать лекарства, то почему медикам можно программировать?». Абстрактному программисту нельзя заняться изобретением и изготовлением лекарств. Причина понятна — у него нет соответствующего образования. А вот с программированием не всё так просто. Кажется, что абстрактный медик, освоив программирование, автоматически принесёт пользу. Благо, научиться как-то программировать проще, чем разобраться в органической химии и принципах создания лекарств.
Здесь кроется подвох. Численный эксперимент требует не меньший аккуратности, чем реальный. Учат мыть пробирки после экспериментов и следят за их стерильностью. Но мало кто всерьез обеспокоен проблемой, что какой-то массив может случайно оказаться неинициализированным.
Программисты знают, что чем сложнее программное обеспечение, тем сложней и не очевидней в нём проявляются ошибки. Другими словами, я говорю о нелинейном росте количества ошибок при росте размера кода. А ведь программы для химических и иных расчётов отнюдь не просты. Здесь и кроется беда. Не страшно, что медик-программист допускает ошибки. Их делает любой программист, независимо от профессионализма. Страшно, что таким результатам начинают всё больше доверять. Посчитали что-то и пошли дальше заниматься своими вещами.
Те, чья основная деятельность программирование, знают опасность такого подхода. Они знают, что такое неопределенное поведение и как программа может делать вид, что выдает правильный результат. Есть статьи и книги, посвященные тому, как правильно писать юнит-тесты и как лучше проверить корректность расчетов.
Это мир программистов. В мире химиков/физиков/медиков, боюсь, дело обстоит не так. Они не пишут сложную программу. Вернее они не думают в этом направлении. Они просто используют компьютер как Большой Калькулятор. Такое сравнение привёл один из читателей. Я приведу его цитату здесь полностью, чтобы после перевода статьи, с ней могли познакомиться и англоязычные читатели.
Имею кое-что сказать по этой теме исходя из личного опыта. Будучи профессиональным программистом, по образованию я — потомственный физик. Так уж вышло, что в тот момент, когда я выбирал ВУЗ, голос крови оказался сильнее, чем вера в светлое будущее IT. И я поступил в достаточно престижную по местным меркам физическую высшую школу, которая, по сути, является «детским садиком» при крупном НИИ в родном городе Нижнем Новгороде. Люди, знающие тему, узнают и НИИ, и название школы.
На протяжении учёбы вполне естественно оказалось, что по части программирования (и в том числе математических методов физического моделирования) я был одним из лучших. И там же выяснились следующие факты:
1. Физики рассматривают компьютер как большой многофункциональный калькулятор, позволяющий построить график зависимости Эта от Тэта при Гамма стремящемся в бесконечность. Причем, очевидным образом, для них цель — график, а вовсе не та программа, которая его рисует.
2. Как следствие из этого факта, программист — это не профессия. Программист — это просто тот человек, который умеет пользоваться Большим Калькулятором, чтобы построить означенный график. Каким способом график будет построен, не имеет значения. Совсем. Как-как вы сказали? Статический анализ? Контроль версий? Окститесь, родные! C++ — язык для программистов. Физики пишут на Фортране!
3. Как следствие из предыдущего пункта, человек, стремящийся посвятить свою жизнь написанию программ физ. моделирования, даже универсальных, даже очень и очень крутых — не более, чем приложение к калькулятору. Он и не человек вовсе, а так… И это, кстати, распространялось не только на меня (куда уж мне, убогому), а даже и на лучшего численника-расчётчика в НИИ, который преподавал у нас численные методы и который, когда я пришел к нему делать курсовую, сказал мне почти открытым текстом: «Вас будут презирать, приготовьтесь терпеть».
Терпеть мне не захотелось, и я после окончания ушел из моделирования в области, где программистов не считают людьми второго сорта. Надеюсь, этот пример объясняет, почему инициативы типа ввода статического анализа даже на сравнительно крупных (до 20-30 человек) проектах по математическому моделированию — гиблое дело. Там просто может не найтись человека, который знает, что это такое. А если такой человек и найдётся, его, скорее всего затопчут, потому что нафиг не нужны эти новомодные программистские прибамбасы. Сто лет без них жили — и дальше проживём.
И тем, кто не заскучал, второй пример. Мой отец, находясь в пенсионном возрасте, тем не менее работает в очень крупном оборонно-инженерном предприятии, здесь же, в Нижнем (самом крупном в городе и одном из крупнейших в стране — те, кто в теме опять же угадают 😉 ). Он всю жизнь программировал на Фортране. Начинал еще с перфокарт. Я не виню его за то, что не учит C++. Ему это уже 10 лет назад было поздно — он еще неплохо держится. Но на предприятии, где 2/3 сотрудников что-то как-то программируют, приняты следующие меры безопасности:
1. Никакого интернета. Совсем. Нужна литература — иди в местную библиотеку. Stack Overflow? А что это? Даже если ты хочешь отправить письмо электронной почтой, ты должен написать заявление начальнику, где ты объяснишь, кому это письмо и для чего. Интернет «под расписку» есть только у избранных. Слава богу, хоть внутренняя сеть есть.
2. Никаких административных прав на рабочем компьютере. Возможно, это правило разумно для офисного планктона, но мне трудно себе представить программиста, которого бы оно устроило.
3. (не относится к делу, просто иллюстрация) Нельзя даже пронести телефон с камерой (а где вы теперь видели другие)?
В результате даже молодняк пишет на Фортране, причем реально грамотных в программировании — единицы. Знаю, потому что подтягивал по программированию одного парня лет 25, который был мне отцом зарекомендован как многообещающий.
Мой вердикт: там 80-е годы. Даже при том, что там неплохо платят, я не пойду туда ни за какие коврижки.
Вот такие два примера из жизни интеллектуальной элиты. Никого не хочу очернить — люди хорошо делают своё дело и так, но иногда смотря, с какими ветряными мельницами порой воюет отец, которого я недавно (слава богу!) смог-таки пересадить в git, сердце сжимается. Никакого ООП в проекте под миллион строк кода, никакого статического анализа.
Просто люди имеют свойство быть очень консервативными в областях, не являющихся их основным «коньком».
(Илья Майзус. Оригинал комментария.)
Самое главное здесь, что компьютер это просто Большой Калькулятор. А раз так, то и знать о нём можно не больше чем заслуживает его младший родственник — «калькулятор карманный». Да, именно так его и используют. В разных областях. Давайте на секунду отвлечемся и заглянем в мир физики. Посмотрим, как находит подтверждение очередная теория. Для этого мне вновь придется привести большую цитату. Источником является книга Брайана Грина «Элегантная вселенная (суперструны, скрытые размерности и поиски окончательной теории)» [1]:
Мы все сгрудились вокруг компьютера Моррисона, стоявшего в нашем кабинете. Аспинуолл объяснил Моррисону, как запустить программу и какой точный вид должны иметь вводимые в нее данные. Моррисон привел полученные ночью результаты к нужному виду, и теперь все было готово.
Расчет, который нужно было провести, грубо говоря, сводился к определению массы конкретной частицы, являющейся колебательной модой струны при ее движении во вселенной, компоненту КалабиЯу которой мы изучали всю осень. Мы надеялись, что в соответствии с выбранной нами стратегией масса окажется точно такой же, что и масса в случае многообразия КалабиЯу, возникшего после флопперестройки с разрывом пространства. Последнюю массу вычислить было легко, и мы сделали это несколькими неделями раньше. Ответ оказался равным 3 в определенной системе единиц, которой мы пользовались. А так как сейчас проводился численный расчет на компьютере, то ожидаемый результат должен был быть близким к числу 3, чтото вроде 3,000001 или 2,999999; отличие от точного ответа объяснялось бы ошибками округления.
Моррисон сел за компьютер. Его палец завис над клавишей «Enter». Напряжение нарастало. Моррисон выдохнул «поехали» и запустил программу. Через пару секунд компьютер выдал ответ: 8,999999. Мое сердце упало. Неужели действительно флопперестройки с разрывом пространства нарушают зеркальную симметрию, а значит, вряд ли существуют в реальности? Но в следующее же мгновение мы сообразили, что здесь какаято глупая ошибка. Если в массах частиц на двух многообразиях действительно есть отличие, почти невероятно, что компьютер выдал бы результат, столь близкий к целому числу. Если наши идеи неверны, то с тем же самым успехом компьютер мог бы выдать ответ, состоящий из совершенно случайных цифр. Мы получили неправильный ответ, но неправильность его была такого вида, из которого напрашивался вывод о том, что гдето мы допустили банальную ошибку. Аспинуолл и я подошли к доске, и моментально ошибка была найдена: мы забыли множитель 3 в «простом» вычислении несколько недель назад, так что правильный результат должен был равняться 9. Поэтому ответ компьютера — это как раз то, на что мы надеялись.
Конечно, совпадение результата после того, как найдена ошибка, является лишь наполовину убедительным. Если известен желаемый результат, очень легко найти способ его получить. Нам срочно требовался другой пример. Имея все необходимые программы, придумать его не представляло сложности. Мы вычислили массу еще одной частицы на верхнем многообразии КалабиЯу, на этот раз с особой тщательностью, чтобы избежать еще одной ошибки. Ответом было число 12. Мы снова окружили компьютер и запустили программу. Через несколько секунд был получен ответ 11,999999. Согласие. Мы доказали, что предполагаемое зеркальное пространство является зеркальным пространством, и флопперестройки с разрывами пространства являются частью теории струн.
Я вскочил со стула и, опьяненный победой, сделал круг по комнате. Моррисон, сияя, сидел за компьютером. И только реакция Аспинуолла была нестандартной. «Здорово. Я и не сомневался, что все так и будет, — спокойно сказал Аспинуолл. — А где мое пиво?»
Я верю, что они гении. Но представим, что таким подходом вычислялось значение интеграла обыкновенными студентами. Но думаю, что тогда программисты посчитали бы такой подход серьезным. А если бы программа сразу выдала 3? Что тогда? Ошибка в программе посчиталась бы за доказательство? Думаю, потом бы ошибка всплыла при перепроверке ими же или другими учеными. Но все равно, «идеальный сферический программист в вакууме» испуган от такого подхода.
Вот такая вот она реальность. Именно так используются не только персональные компьютеры, но и кластерные системы в научных вычислениях. А самое главное, люди доверяют результатам работы программ. И чем дальше, тем больше таких вычислении будет. И тем больше будет опасность существования ошибок в их коде.
Быть может пора что-то менять?
Я имею права сам себе наклеить пластырь. Могу порекомендовать, что, по моему мнению, стоит выпить при простуде. Но не более. Я не могу сверлить зуб или выписать рецепт.
Быть может, когда ответственность создаваемой программной системы выходит за определённые рамки, её разработчики тоже должны подтверждать свою квалификацию?
Я знаю про различные сертификации. Но я говорю о другом. Сертификация направлена на то, чтобы код программ соответствовал определенным нормам. Косвенно, это частично защищает от халтуры. Однако, список сфер, где требуется сертификация достаточно узок. Он явно не покрывает весь спектр, где неаккуратное обращение с Большим Калькулятором может навредить.
Пример опасности
Думаю, многим мои переживания кажутся слишком абстрактами. Поэтому, давайте рассмотрим что-то из практики. Есть открытый пакет Trans-Proteomic Pipeline (TPP), для решении задач в сфере биологии. Его явно используют. Используют его те, кто разрабатывает и возможно сторонние организации. Мне кажется, наличие любой в нем ошибки, уже потенциальная проблема. А есть ли в нём ошибки? Да есть. И появляются всё новые. Год назад, мы проверяли этот проект и написали заметку "Проверка проекта Trans-Proteomic Pipeline (TPP)".
Изменилось что-то с тех пор? Ничего не изменилось. Проект продолжает развиваться и обрастать новыми ошибками. Большой Калькулятор победил. Разработчики не заняты написанием высококачественного проекта с минимально возможным количеством ошибок. Они просто решают задачи. Будь иначе, они бы как-то отреагировали на предыдущую статью и задумались над внедрением каких-то инструментов статического анализа. Я не имею в виду, что они были обязаны выбрать PVS-Studio. Есть много других статических анализаторов кода. Важно то, что в ответственном приложении продолжают появляться типичнейшие ошибки. Давайте посмотрим, что есть новенького.
1. Какой-то неумеха продолжает писать неправильные циклы
В предыдущей статье, я уже писал про некорректные условия в циклах. Есть такие ошибки и новой версии пакета.
double SpectraSTPeakList::calcDot(SpectraSTPeakList* other) { .... for (i = this->m_bins->begin(), j = other->m_bins->begin(); i != this->m_bins->end(), j != other->m_bins->end(); i++, j++) { d = (*i) * (*j); dot += d; } .... }Диагностическое сообщение PVS-Studio: V521 Such expressions using the ‘,’ operator are dangerous. Make sure the expression is correct. spectrastpeaklist.cpp 504
В проверке «i != this->m_bins->end(), j != other->m_bins->end()», выражение стоящее до запятой ничего не проверяет. Оператор запятая ‘,’ используется для выполнения стоящих по обе стороны от него выражений в порядке слева направо и возвращает значение правого выражения. Корректная проверка должна выглядеть так:
i != this->m_bins->end() && j != other->m_bins->end()Аналогичные ляпы можно увидеть здесь:
- spectrastpeaklist.cpp 516
- spectrastpeaklist.cpp 529
- spectrastpeaklist.cpp 592
- spectrastpeaklist.cpp 608
- spectrastpeaklist.cpp 625
- spectrastpeaklist.cpp 696
2. Разыменовывание нулевого указателя
Такая ошибка не приведет к неправильным результатам вычислений. Будет падание программы, что намного лучше. Однако, не написать про эти ошибки будет тоже странно.
void ASAPRatio_getDataStrctRatio(dataStrct *data, ....) { .... int *outliers, *pepIndx=NULL; .... //pepIndx не изменяется .... if(data->dataCnts[i] == 1 && pepIndx[i] == 0) data->dataCnts[i] = 0; .... }Диагностическое сообщение PVS-Studio: V522 Dereferencing of the null pointer ‘pepIndx’ might take place. asapcgidisplay2main.cxx 534
Здесь также разыменовывается нулевой указатель:
- Pointer ‘peptides’. asapcgidisplay2main.cxx 556
- Pointer ‘peptides’. asapcgidisplay2main.cxx 557
- Pointer ‘peptides’. asapcgidisplay2main.cxx 558
- Pointer ‘peptides’. asapcgidisplay2main.cxx 559
- Pointer ‘peptides’. asapcgidisplay2main.cxx 560
- Pointer ‘pepIndx’. asapcgidisplay2main.cxx 569
3. Неочищенные массивы
static void clearTagNames() { std::vector<const char *>ptrs; for (tagname_set::iterator i = tagnames.begin(); i!=tagnames.end();i++) { ptrs.push_back(*i); } for (tagname_set::iterator j = attrnames.begin(); j!=attrnames.end();j++) { ptrs.push_back(*j); } tagnames.empty(); attrnames.empty(); for (size_t n=ptrs.size();n--;) { delete [] (char *)(ptrs[n]); // cast away const } }Анализатор заметил здесь сразу два неочищенных массива:
V530 The return value of function ’empty’ is required to be utilized. tag.cxx 72
V530 The return value of function ’empty’ is required to be utilized. tag.cxx 73
Вместо функции empty() следует вызывать функцию clear().
4. Неинициализированные объекты классов
class ExperimentCycleRecord { public: ExperimentCycleRecord() { ExperimentCycleRecord(0,0,0,True,False); } ExperimentCycleRecord(long lExperiment, long lCycleStart, long lCycleEnd, Boolean bSingleCycle, Boolean bRangleCycle) { .... } .... }Диагностическое сообщение PVS-Studio: V603 The object was created but it is not being used. If you wish to call constructor, ‘this->ExperimentCycleRecord::ExperimentCycleRecord(….)’ should be used. mascotconverter.cxx 101
Конструктор ExperimentCycleRecord() не выполняет своего предназначения. Он ничего не инициализирует. Разработчик может быть хорошим химиком, но если он не знает, как работать с языком Си++, то грош цена вычислениям, в которых используется неинициализированная память. Это как взять грязную пробирку.
Строчка «ExperimentCycleRecord(0,0,0,True,False);» место вызова другого конструктора, создает временный объект, который будет разрушен. Подробнее этот паттерн ошибки я рассматривал в статье "Не зная брода, не лезь в воду — часть первая".
Аналогичные неправильные конструкторы можно найти здесь:
- asapratiopeptideparser.cxx 57
- asapratiopeptidecgidisplayparser.cxx 36
- cruxdiscrimfunction.cxx 36
- discrimvalmixturedistr.cxx 34
- mascotdiscrimfunction.cxx 47
- mascotscoreparser.cxx 37
- tandemdiscrimfunction.cxx 35
- tandemkscoredf.cxx 37
- tandemnativedf.cxx 37
5. Комментарии, которые нарушили логику
int main(int argc, char** argv) { .... if (getIsInteractiveMode()) //p->writePepSHTML(); //p->printResult(); // regression test? if (testType!=NO_TEST) { TagListComparator("InterProphetParser",testType, outfilename,testFileName); .... }Диагностическое сообщение PVS-Studio: V628 It’s possible that the line was commented out improperly, thus altering the program’s operation logics. interprophetmain.cxx 175
После оператора ‘if’ были закомментированы строчки, выполняющие действия. В результате, логика работы программа изменилась не так, как планировал программист. Он хотел, чтобы при выполнении условия ничего не происходило. Вместо этого, оператор ‘if’ оказывает влияние на код, расположенный ниже. Результат — запуск тестов зависит теперь не только от условия «testType!=NO_TEST», но и от условия «getIsInteractiveMode()». Тест может ничего не тестировать. Крайне рекомендую не полагаться полностью на одну методологию тестирования (например, TDD).
6. Опечатки
Опечатки есть везде и всегда. Не страшно, если из-за такой ошибки в игре у вас станет меньше жизней после взрыва, чем планировалось. А что означают неправильные данные при химических расчетах?
void ASAPRatio_getProDataStrct(proDataStrct *data, char **pepBofFiles) { .... if (data->indx == -1) { data->ratio[0] = -2.; data->ratio[0] = 0.; data->inv_ratio[0] = -2.; data->inv_ratio[1] = 0.; return; } .... }Диагностическое сообщение PVS-Studio: V519 The ‘data->ratio[0]’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 130, 131. asapcgidisplay2main.cxx 131
Случайно два раза записали значения в одну и ту же переменную. Должно было быть:
data->ratio[0] = -2.; data->ratio[1] = 0.;А потом, ещё копировали этот фрагмент в другие местам программы:
- asapcgidisplay2main.cxx 338
- asapcgidisplay2main.cxx 465
- asapratioproteincgidisplayparser.cxx 393
- asapratioproteincgidisplayparser.cxx 518
7. Сравнение знаковых и беззнаковых чисел.
Сравнивать знаковые и беззнаковые числа надо уметь. В обыкновенных калькуляторах нет беззнаковых чисел. А в языке Си++ есть.
size_type size() const; void computeDegenWts() { .... int have_cluster = 0; .... if ( have_cluster > 0 && ppw_ref.size() - have_cluster > 0 ) .... }Диагностическое сообщение PVS-Studio: V555 The expression ‘ppw_ref.size() — have_cluster > 0’ will work as ‘ppw_ref.size() != have_cluster’. proteinprophet.cpp 6767
Хотелось выполнить проверку «ppw_ref.size() > have_cluster». Но получилось совсем иное.
Для простоты, пусть у нас тип ‘size_t’ будет 32-битным. Предположим, функция «ppw_ref.size()» вернёт число 10, а переменная have_cluster равна 15. Функция ppw_ref.size() возвращает беззнаковый тип ‘size_t’. По правилам языка Си++ перед вычитанием правый оператор в операции минус тоже станет иметь тип ‘size_t’. Пока всё хорошо. Слева у нас 10u, справа 15u.
Вычитаем:
10u — 15u
А вот здесь беда. Всё те же правил языка Си++ говорят, что результат вычитания двух переменных беззнакового типа, также будет иметь беззнаковый тип.
Это значит, что 10u — 15u = FFFFFFFBu. А, как известно, 4294967291 больше 0.
Бунт Большого Калькулятора удаётся. Мало написать правильный теоретический алгоритм. Надо ещё написать правильный код.
Схожая по духу ошибка присутствует здесь:
double SpectraSTPeakList::calcXCorr() { .... for (int tau = -75; tau <= 75; tau++) { float dot = 0.0; for (unsigned int b = 0; b < numBins; b++) { if (b + tau >= 0 && b + tau < (int)numBins) { dot += (*m_bins)[b] * theoBins[b + tau] / 10000.0; } } .... .... }Диагностическое сообщение PVS-Studio: V547 Expression ‘b + tau >= 0’ is always true. Unsigned type value is always >= 0. spectrastpeaklist.cpp 2058
Как видно из кода, переменная ‘tau’ принимает значения в диапазоне [-75, 75]. Чтобы не выйти за границу массива имеется проверка: b + tau >= 0. Я думаю, вы уже поняли, что эта проверка не работает. Переменная ‘b’ имеет тип ‘unsigned’. Это значит, что и выражение «b + tau» имеет тип unsigned. Значение типа unsigned всегда больше или равно 0.
8. Странный цикл
const char* ResidueMass::getStdModResidues(....) { .... for (rmap::const_iterator i = p.first; i != p.second; ++i) { const cResidue &r = (*i).second; if (r.m_masses[0].m_nterm) { n_term_aa_mod = true; } else if (r.m_masses[0].m_cterm) { c_term_aa_mod = true; } return r.m_residue.c_str(); } if(! strcmp(mod, "+N-formyl-met (Protein)")) { return "n"; } if (! strcmp(mod, "13C6-15N2 (K)")) { return "K"; } if (! strcmp(mod, "13C6-15N4 (R)")) { return "R"; .... }Предупреждение выданное PVS-Studio: V612 An unconditional ‘return’ within a loop. residuemass.cxx 1442
Внутри цикла есть оператор ‘return’, который вызывается в любом случае. Цикл может выполнить только одну итерацию, после чего функция завершится. Думаю, здесь или опечатка или не хватает условия перед оператором ‘return’.
9. Грубые вычисления
double RTCalculator::getUsedForGradientRate() { if (rts_.size() > 0) return used_count_ / rts_.size(); return 0.; }Предупреждение выданное PVS-Studio: V636 The ‘used_count_ / rts_.size()’ expression was implicitly casted from ‘int’ type to ‘double’ type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. rtcalculator.cxx 6406
Так как функция возвращает значения типа double, то разумно предположить следующее.
Если переменная ‘used_count_’ равна 5, а функция rts_.size() вернет 7, то результат должен быть приблизительно равен 0,714. Вот только функция getUsedForGradientRate() в этом случае вернет 0.
Переменная ‘used_count_’ имеет тип int. Функция rts_.size() тоже возвращает значение типа ‘int’. Происходит целочисленное деление. Результат очевиден. Он равен нулю. Затем ноль неявно приводится к типу double. Но это уже не важно.
Чтобы исправить ситуацию, можно написать так:
return static_cast<double>(used_count_) / rts_.size();Аналогичные недочеты:
- cgi_pep3d_xml.cxx 3203
- cgi_pep3d_xml.cxx 3204
- asapratiopeptideparser.cxx 4108
10. Великий и могучий Copy-Paste
Функция setPepMaxProb() содержит несколько больших однотипных блоков. Сразу чувствуется, что без методики Copy-Paste здесь не обошлось. А как результат, код содержит ошибку. Мне пришлось ОЧЕНЬ сильно сократить код примера. В сокращенном варианте ошибка легко видна. В коде программы, заметить её почти не реально. Да, это реклама инструментов статического анализа вообще, и PVS-Studio в частности.
void setPepMaxProb( bool use_nsp, bool use_fpkm, bool use_joint_probs, bool compute_spectrum_cnts ) { double prob = 0.0; double max2 = 0.0; double max3 = 0.0; double max4 = 0.0; double max5 = 0.0; double max6 = 0.0; double max7 = 0.0; .... if ( pep3 ) { ... if ( use_joint_probs && prob > max3 ) ... } .... if ( pep4 ) { ... if ( use_joint_probs && prob > max4 ) ... } .... if ( pep5 ) { ... if ( use_joint_probs && prob > max5 ) ... } .... if ( pep6 ) { ... if ( use_joint_probs && prob > max6 ) ... } .... if ( pep7 ) { ... if ( use_joint_probs && prob > max6 ) ... } .... }V525 The code containing the collection of similar blocks. Check items ‘max3’, ‘max4’, ‘max5’, ‘max6’, ‘max6’ in lines 4664, 4690, 4716, 4743, 4770. proteinprophet.cpp 4664
Предупреждение выданное PVS-Studio: V525 The code containing the collection of similar blocks. Check items ‘max3’, ‘max4’, ‘max5’, ‘max6’, ‘max6’ in lines 4664, 4690, 4716, 4743, 4770. proteinprophet.cpp 4664
К сожалению диагностика V525 даёт много ложных срабатываний и отнесена к третьему уровню предупреждений. Но если не полениться их изучать, можно находить вот такие замечательные баги.
11. Указатель не всегда инициализируется
int main(int argc, char** argv) { .... ramp_fileoffset_t *pScanIndex; .... if ( (pFI=rampOpenFile(mzXmlPath_.c_str()))==NULL) { .... } else { .... pScanIndex = readIndex(pFI, indexOffset, &iAnalysisLastScan); .... } .... if (pScanIndex != NULL) free(pScanIndex); return 0; }Предупреждение выданное PVS-Studio: V614 Potentially uninitialized pointer ‘pScanIndex’ used. sqt2xml.cxx 476
Эта программа может в конце аварийно завершиться, если функция rampOpenFile() вернёт NULL. Не критично, но неприятно.
Другая переменная, которая может оказаться не инициализированной, находится здесь:
- Potentially uninitialized pointer ‘fp_’ used. dta-xml.cpp 307
12. Нет виртуального деструктора
class DiscriminantFunction { public: DiscriminantFunction(int charge); virtual Boolean isComputable(SearchResult* result) = 0; virtual double getDiscriminantScore(SearchResult* result) = 0; virtual void error(int charge); protected: int charge_; double const_; }; // class class CometDiscrimFunction : public DiscriminantFunction; class CruxDiscrimFunction : public DiscriminantFunction; class InspectDiscrimFunction : public DiscriminantFunction; ..... class DiscrimValMixtureDistr : public MixtureDistr { .... DiscriminantFunction* discrim_func_; .... }; DiscrimValMixtureDistr::~DiscrimValMixtureDistr() { delete[] posinit_; delete[] neginit_; delete discrim_func_; }Предупреждение выданное PVS-Studio: V599 The virtual destructor is not present, although the ‘DiscriminantFunction’ class contains virtual functions. discrimvalmixturedistr.cxx 206
От класса DiscriminantFunction наследуется множество классов. Например, наследником является класс DiscrimValMixtureDistr. Деструктор этого класса освобождает память, а, следовательно, очень желательно его вызывать. К сожалению, деструктор в классе DiscriminantFunction не объявлен виртуальным, со всеми вытекающими последствиями.
13. Разное
Можно найти массу недочетов, которые не приведут к серьезным последствиям, но их присутствие в коде неприятно. Есть и просто подозрительные, но не обязательно ошибочные места. Вот одно из них:
Boolean MixtureModel::iterate(int counter) { .... if (done_[charge] < 0) { done_[charge]; } else if (priors_[charge] > 0.0) { done_[charge] += extraitrs_; } .... }Предупреждение выданное PVS-Studio: V607 Ownerless expression ‘done_[charge]’. mixturemodel.cxx 1558
Что это? Недописанный код? Или хотели подчеркнуть, что не надо ничего делать, если выполнится условие «done_[charge] < 0»?
А вот неправильное освобождение памяти. С большой вероятностью страшных последствий не будет, но этот код с запахом.
string Field::getText(....) { .... char* pepString = new char[peplen + 1]; .... delete pepString; .... }Предупреждение выданное PVS-Studio: V611 The memory was allocated using ‘new T[]’ operator but was released using the ‘delete’ operator. Consider inspecting this code. It’s probably better to use ‘delete [] pepString;’. pepxfield.cxx 1023
Здесь следовало написать «delete [] pepString». Такое место далеко не одно:
- cruxdiscrimvalmixturedistr.cxx 705
- cruxdiscrimvalmixturedistr.cxx 715
- mascotdiscrimvalmixturedistr.cxx 426
- mascotdiscrimvalmixturedistr.cxx 550
- mascotdiscrimvalmixturedistr.cxx 624
- phenyxdiscrimvalmixturedistr.cxx 692
- probiddiscrimvalmixturedistr.cxx 487
- probiddiscrimvalmixturedistr.cxx 659
- tandemdiscrimvalmixturedistr.cxx 731
- tandemdiscrimvalmixturedistr.cxx 741
А вот реализация оператора "—". Видимо он не используется нигде. Иначе ошибка бы быстро выявилась.
CharIndexedVectorIterator operator++(int) { // postincrement CharIndexedVectorIterator _Tmp = *this; ++m_itr; return (_Tmp); } CharIndexedVectorIterator& operator--() { // predecrement ++m_itr; return (*this); }Предупреждение выданное PVS-Studio: V524 It is odd that the body of ‘—‘ function is fully equivalent to the body of ‘++’ function. charindexedvector.hpp 81
Операторы "—" и "++" реализованы одинаково. А дальше, наверное, скопировали:
- charindexedvector.hpp 87
- charindexedvector.hpp 159
- charindexedvector.hpp 165
Продолжать дальше не буду. Это не так уж всё интересно, да и статья затянулась. Как всегда прошу разработчиков, не останавливаться на правке перечисленных недоработок. Скачайте и проверьте самостоятельно проект с помощью PVS-Studio. Я мог пропустить многие ошибки. Мы готовы выделить бесплатный ключ на некоторое время.
Резюмирую
К сожалению, статья получилась путанной. Так что же хотел сказать автор? Попробую совсем кратко повторить, какие мысли я хотел донести.
- Мы все больше используем и доверяем программам, выполняющим расчёты и моделирующие процессы.
- Программы сильно усложняются. Профессиональным программистам очевидно, что нельзя создавать пакет численного моделирования так же, как использовать программируемый калькулятор. Рост сложности ведёт к экспоненциальному росту количества ошибок [2].
- Получается, что физики/биологи/медики уже не могут по-прежнему просто что-то посчитать. Нельзя игнорировать рост сложности программ и последствия неправильных вычислений в силу неполного владения языком программирования.
- В статье я привел аргументы, что всё обстоит именно так. Первая цитата — люди используют компьютер, как калькулятор. Вторая цитата — да, именно как калькулятор. Примеры ошибок — да, и действительно допускают ошибок. Мои опасения обоснованы.
И что предлагается делать?
Для начала я хочу, чтобы люди начали осознавать эту проблему. И рассказывали знакомым из смежных областей о ней. Для программистов давно очевидно, что рост сложности и глупые ошибки в больших проектах, легко доводят до беды. Люди же, использующие программирование просто как инструмент, не знают про это и не задумываются. И им стоит подсказать обратить на это внимание.
Аналогия. Люди взяли в руку дубину и начали охотиться за зверьем. Пока они этим занимаются, оружие стремительно совершенствуется. Дубина в их руках превращается в каменный молоток, потом в меч, потом в ружьё. А они по прежнему стараются ей просто глушить зайцев по голове. Мало того, что это неэффективно, так ещё и стало намного опасней (можно в себя выстрелить или в коллегу). Охотники из племени «программистов» быстро приспосабливаются. А остальным некогда. Они заняты охотой за зайцами. Ведь смысл именно в зайцах. Надо подсказать этим людям, что хотят они того или нет, им надо или учиться. Так только всем лучше будет. Нечего ружьем махать.
Дополнительные ссылки
- Брайан Грин «Элегантная вселенная (суперструны, скрытые размерности и поиски окончательной теории. ISBN 978-5-453-00011-1 (УРСС), ISBN 978-5-397-01575-2 (Книжный дом „ЛИБРОКОМ“)
- Андрей Карпов. Ощущения, которые подтвердились числами. http://www.viva64.com/ru/b/0158/
ссылка на оригинал статьи http://habrahabr.ru/company/pvs-studio/blog/192624/
Добавить комментарий