Как уменьшить вероятность ошибки на этапе написания кода. Заметка N3

    PVS-Studio VS QT
    Это третья статья, где я хочу рассказать про новую пару приёмов при программировании, которые помогут сделать код более простым и надежным. С предыдущими двумя заметками можно познакомиться здесь [1] и здесь [2]. В этот раз примеры будут взяты из проекта Qt.

    Введение


    Проект Qt 4.7.3. попался мне для изучения не случайно. Пользователи PVS-Studio обратили внимание, что анализ как-то слабоват, если дело касается проверки проектов, построенных на основе библиотеки Qt. Это не удивительно. Статический анализ позволяет находить ошибки за счет того, что смотрит на код более высокоуровнево, чем компилятор. Следовательно, он должен знать определенные паттерны кода и, что делают функции различных библиотек. В противном случае, он пройдет мимо многих замечательных ляпов. Поясню на примере:
    if (strcmp(My_Str_A, My_Str_A) == 0)

    Бессмысленно сравнивать строку саму с собой. Но компилятор промолчит. Он не задумывается, в чем суть функции strcmp(). У него хватает своих забот. А вот статические анализаторы могут заподозрить неладное. В Qt есть своя разновидность функции сравнения строк — qstrcmp(). И, соответственно, анализатор нужно обучить обращать внимание и на такую строку:
    if (qstrcmp(My_Str_A, My_Str_A) == 0)

    Осваивание библиотеки Qt и создание специализированных проверок — это большая и планомерная работа. И началом этой работы стала проверка самой библиотеки.

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

    1. Обрабатывайте переменные в той же последовательности, как они объявлены


    Код библиотеки Qt очень качественен и практически не содержит ошибок. Зато в нём обнаружилось большое количество излишних инициализаций, излишних сравнений или лишних копирования значений переменных.

    Приведу пару примеров для ясности:
    QWidget *WidgetFactory::createWidget(...)
    {
      ...
      } else if (widgetName == m_strings.m_qDockWidget) { <<<===
        w = new QDesignerDockWidget(parentWidget);            
      } else if (widgetName == m_strings.m_qMenuBar) {
        w = new QDesignerMenuBar(parentWidget);
      } else if (widgetName == m_strings.m_qMenu) {
        w = new QDesignerMenu(parentWidget);
      } else if (widgetName == m_strings.m_spacer) {
        w = new Spacer(parentWidget);
      } else if (widgetName == m_strings.m_qDockWidget) { <<<===
        w = new QDesignerDockWidget(parentWidget);
      ...
    }

    Здесь два раза продублировано одно и то же сравнение. Это не ошибка, но совершенно избыточный код. Другой аналогичный пример:
    void QXmlStreamReaderPrivate::init()
    {
      tos = 0;  <<<===
      scanDtd = false;
      token = -1;
      token_char = 0;
      isEmptyElement = false;
      isWhitespace = true;
      isCDATA = false;
      standalone = false;
      tos = 0;  <<<===
      ...
    }

    Вновь не ошибка, но совершенно ненужная двойная инициализация переменной. И подобных продублированных действий я насчитал очень много. Возникают они из-за длинных списков сравнений, присваиваний, инициализаций. Просто не видно, что переменная уже обрабатывается, из-за чего и появляются лишние операции. Я могу назвать три неприятных последствия наличия таких дублирующихся действий:
    1. Дубликаты увеличивают длину кода. А чем длиннее код, тем легче добавить еще один дубликат.
    2. Если мы захотим изменить логику программы и удалим одну проверку или одно присваивание, то дубликат этого действия может подарить вам несколько часов увлекательной отладки. Сами представьте, вы пишите «tos = 1» (см. первый пример), а потом в другой части программы удивляетесь, отчего же по-прежнему «tos» равен нулю.
    3. Замедление скорости работы. Практически всегда им можно пренебречь в таких ситуациях, но оно всё-таки есть.

    Надеюсь, убедил, что дубликатам не место в нашем коде. Как с ними бороться? Как правило, подобные инициализации/сравнения идут блоком. И есть такой же блок переменных. Рационально писать код так, чтобы порядок объявлений переменных и порядок работы с ними совпадал. Приведу пример не очень хорошего кода:
    struct T {
      int x, y, z;
      float m;
      int q, w, e, r, t;
    } A;
    ...
    A.m = 0.0;
    A.q = 0;
    A.x = 0;
    A.y = 0;
    A.z = 0;
    A.q = 0;
    A.w = 0;
    A.r = 1;
    A.e = 1;
    A.t = 1;

    Естественно, это схематичный пример. Смысл в том, что когда инициализация не последовательна, то намного легче написать две одинаковых строки. В приведенном коде дважды инициализируется переменная 'q'. И ошибка плохо заметна, если просматривать код бегло. Если теперь инициализировать переменные в той же последовательности, как они объявлены, то подобной ошибке просто не будет места. Улучшенный вариант кода:
    struct T {
      int x, y, z;
      float m;
      int q, w, e, r, t;
    } A;
    ...
    A.x = 0;
    A.y = 0;
    A.z = 0;
    A.m = 0.0;
    A.q = 0;
    A.w = 0;
    A.e = 1;
    A.r = 1;
    A.t = 1;

    Конечно, я знаю, что не всегда так можно написать и работать с переменными в порядке их объявления. Но часто это возможно и полезно. Дополнительным плюсом станет то, что вам легче будет ориентироваться в коде.

    Рекомендация. Добавляя новую переменную, постарайтесь, чтобы её инициализация и обработка, осуществлялась в соответствии с её положением относительно других переменных.

    2. Табличные методы — это хорошо.


    Про табличные методы хорошо написано у С. Макконнелла в книге «Совершенный код» в главе N18 [3]:

    Табличный метод — это схема, позволяющая искать информацию в таблице, а не использовать для этого логические выражения, такие как if и case. Практически все, что вы можете выбирать посредством логических операторов, можно выбирать, применяя таблицы. В простых случаях логические выражения проще и понятней. Но при усложнении логических построений таблицы становятся всё привлекательнее.

    Так вот, очень жаль, что программисты по-прежнему любят огромные switch() или густой лес конструкций if-else. Перебороть в себе это очень сложно. Кажется, ну еще-то один «case:» или маленький «if» не повредит. Но он вредит. И неудачно добавляют новые условия даже самые опытные программисты. В качестве примера пара дефектов, найденных в Qt.
    int QCleanlooksStyle::pixelMetric(...)
    {
      int ret = -1;
      switch (metric) {
        ...
        case PM_SpinBoxFrameWidth:
          ret = 3;
          break;
        case PM_MenuBarItemSpacing:
          ret = 6;
        case PM_MenuBarHMargin:
          ret = 0;
          break;
        ...
    }

    Длинный-предлинный switch(). И, естественно, имеется забытый оператор «break». Анализатор выявил эту ошибку за счет того, что переменной «ret» дважды подряд присваивается разное значение.

    Пожалуй, намного лучше, было бы завести какой-то std::map<PixelMetric, int> и явно табличкой задать соответствие между метрикой и числами. Можно придумать и другие табличные варианты реализации функции.

    Еще один пример:
    QStringList ProFileEvaluator::Private::values(...)
    {
      ...
      else if (ver == QSysInfo::WV_NT)
        ret = QLatin1String("WinNT");
      else if (ver == QSysInfo::WV_2000)
        ret = QLatin1String("Win2000");
      else if (ver == QSysInfo::WV_2000)  <<<=== 2003
        ret = QLatin1String("Win2003");
      else if (ver == QSysInfo::WV_XP)
        ret = QLatin1String("WinXP");
      ...
    }

    В коде два раза сравниваем переменную 'ver' с константой WV_2000. Хороший пример, где самое место табличному методу. Например, такой метод мог бы выглядеть так:
    struct {
      QSysInfo::WinVersion; m_ver;
      const char *m_str;
    } Table_WinVersionToString[] = {
      { WV_Me,   "WinMe" },
      { WV_95,   "Win95" },
      { WV_98,   "Win98" },
      { WV_NT,   "WinNT" },
      { WV_2000, "Win2000" },
      { WV_2003, "Win2003" },
      { WV_XP,   "WinXP" },
      { WV_VISTA,"WinVista" }
    };
    
    ret = QLatin1String("Unknown");
    for (size_t i = 0; i != count_of(Table_WinVersionToString); ++i)
      if (Table_WinVersionToString[i].m_ver == ver)
        ret = QLatin1String(Table_WinVersionToString[i].m_str);

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

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

    3. Разное интересное


    Поскольку Qt большая библиотека, то, несмотря на высокое качество, в ней можно встретить разнообразнейшие ошибки. Действует закон больших чисел. Размер *.cpp, *.h и аналогичных файлов проекта Qt составляет около 250 мегабайт. Как не маловероятна ошибка, в большом коде её встретить вполне реально. На основании других ошибок, которые я обнаружил в Qt, составить какие-то рекомендации сложно. Просто опишу некоторые ошибки, которые мне понравились.
    QString decodeMSG(const MSG& msg)
    {
      ...
      int repCount     = (lKeyData & 0xffff);        // Bit 0-15
      int scanCode     = (lKeyData & 0xf0000) >> 16; // Bit 16-23
      bool contextCode = (lKeyData && 0x20000000);   // Bit 29
      bool prevState   = (lKeyData && 0x40000000);   // Bit 30
      bool transState  = (lKeyData && 0x80000000);   // Bit 31
      ...
    }

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

    Следующий пример будет на тему длинных выражений:
    static ShiftResult shift(...)
    {
      ...
      qreal l = (orig->x1 - orig->x2)*(orig->x1 - orig->x2) +
                (orig->y1 - orig->y2)*(orig->y1 - orig->y1) *
                (orig->x3 - orig->x4)*(orig->x3 - orig->x4) +
                (orig->y3 - orig->y4)*(orig->y3 - orig->y4);
      ...
    }

    Видите ошибку? Вот-вот, с ходу и не заметишь. Хорошо, подскажу. Беда вот здесь: «orig->y1 — orig->y1». Ещё меня третье умножение смущает, но возможно так и надо.

    Да, еще вопрос. А у вас ведь тоже в программах вот такие вот блоки вычислений есть? Не пора ли попробовать статический анализатор кода PVS-Studio? Так, порекламировал. Пойдем дальше.

    Использование неинициализированных переменных. Их можно найти в любом большом приложении:
    PassRefPtr<Structure> 
    Structure::getterSetterTransition(Structure* structure)
    {
      ...
      RefPtr<Structure> transition = create(
        structure->storedPrototype(), structure->typeInfo());
      transition->m_propertyStorageCapacity = structure->m_propertyStorageCapacity;
      transition->m_hasGetterSetterProperties = transition->m_hasGetterSetterProperties;
      transition->m_hasNonEnumerableProperties = structure->m_hasNonEnumerableProperties;
      transition->m_specificFunctionThrashCount = structure->m_specificFunctionThrashCount;
      ...
    }

    Здесь опять нужно дать подсказу, чтобы долго не мучить ваши глаза. Смотреть надо на инициализацию переменной «transition->m_hasGetterSetterProperties».

    Уверен, почти каждый из нас, когда только начинал программировать, делал ошибку в духе:
    const char *p = ...;
    if (p == "12345")

    И только потом приходило осознание, зачем нужны такие на первый взгляд странные функции, как strcmp(). К сожалению, язык Си++ настолько суров, что сделать такую ошибку можно и через много лет, будучи профессиональным разработчиком с опытом:
    const TCHAR* getQueryName() const;
    ...
    Query* MultiFieldQueryParser::parse(...)
    {
      ...
      if (q && (q->getQueryName() != _T("BooleanQuery") ...
      ...
    }

    Что бы еще показать. Вот, например, неправильно написанный обмен значений переменных.
    bool qt_testCollision(...)
    {
      ...
      t=x1; x1=x2; x2=t;
      t=y1; x1=y2; y2=t;
      ...
    }

    Это пример того, что можно ошибиться даже в очень простом коде. Так, пока ещё не было примеров на тему выхода за границы массива. Сейчас будет:
    bool equals( class1* val1, class2* val2 ) const
    {
      ...
      size_t size = val1->size();
      ...
      while ( --size >= 0 ){
        if ( !comp(*itr1,*itr2) )
          return false;
        itr1++;
        itr2++;
      }
      ...
    }

    Условие "--size >= 0" всегда истинно, так как переменная size имеет беззнаковый тип. Если будут сравниваться одинаковые последовательности, то произойдет выход за границы массивов.

    Можно продолжать и дальше. Надеюсь, вы как программисты понимаете, что ошибки в проекте такого объема описывать в одной статье нет никакой возможности. Поэтому последнее, на закуску:
    STDMETHODIMP QEnumPins::QueryInterface(const IID &iid,void **out)
    {
      ...
      if (S_OK)
        AddRef();
      return hr;
    }

    Должно было быть что-то в духе «if (hr == S_OK)» или «if (SUCCEEDED(hr))». Макрос S_OK есть не что иное, как 0. Поэтому бяка с неправильным подсчетом количества ссылок здесь неизбежна.

    Вместо заключения


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

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

    Библиографический список


    1. Андрей Карпов. Как уменьшить вероятность ошибки на этапе написания кода. Заметка N1. http://habrahabr.ru/blogs/cpp/115143/
    2. Андрей Карпов. Как уменьшить вероятность ошибки на этапе написания кода. Заметка N2. http://habrahabr.ru/blogs/cpp/116397/
    3. Макконнелл С. Совершенный код. Мастер-класс / Пер. с англ. — М.: Издательско-торговый дом «Русская Редакция»; СПб.: Питер, 2005. — 896 стр.: ил.
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

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

      +6
      int scanCode = (lKeyData & 0xf0000) >> 16; // Bit 16-23

      На самом деле, в этой строке, судя по комментарию, две ошибки :)
        +1
        Видимо да. Кстати, это не единственно место с неудачным оператором &&. Вот этот пример в статью не вошёл, но он тоже прекрасен:
        
        #define WAVE_FORMAT_1M08       0x00000001       /* 11.025 kHz, Mono,   8-bit  */
        #define WAVE_FORMAT_1S08       0x00000002       /* 11.025 kHz, Stereo, 8-bit  */
        ... и так далее
        
        void QAudioDeviceInfoInternal::updateLists()
        {
          ...
          DWORD fmt;
          ...
          if((fmt && WAVE_FORMAT_1M08)
             || (fmt && WAVE_FORMAT_1S08)
             || (fmt && WAVE_FORMAT_2M08)
             || (fmt && WAVE_FORMAT_2S08)
             || (fmt && WAVE_FORMAT_4M08)
             || (fmt && WAVE_FORMAT_4S08)
          #ifndef Q_OS_WINCE
             || (fmt && WAVE_FORMAT_48M08)
             || (fmt && WAVE_FORMAT_48S08)
             || (fmt && WAVE_FORMAT_96M08)
             || (fmt && WAVE_FORMAT_96S08)
          #endif
          )
          ...
        }

        0
        А можно узнать, были ли разработчики Qt уведомлены о найденных ошибках?
          +4
          Нет. Я имею на руках здоровенный отчет, который нужно внимательно исследовать. Я отправлю разработчикам QT ссылку на перевод этой статьи, когда он появится. Если их заинтересует эта заметка, они сами смогут проверить библиотеку и исправить то, что посчитают дефектами.
        –18
        > Как уменьшить вероятность ошибки на этапе написания кода

        Не юзать сраный Си++)
          +7
          Прям тролль:) Судя по вашему профилю, вам нравится C и Перл. А что с С++? Откудо такая ненависть не смогли переучиться, классов боитесь? По собственному опыту, на Перле можно куда круче накосячить.
          +1
          Есть довольно дорогой способ проверки качества кода — заставлять одного (лучше более опытного, но не обязательно) программиста смотреть правки другого и давать замечания. Для того, чтобы замечания были обоснованными — также необходимо наличие некоторого корпоративного свода правил кодирования.
            0
            Да, слишком дорого. Поэтому и появился компромисс — статические анализаторы исходного кода. Ими может воспользоваться сам программист. А может и тот, кто просматривает чужой код. Он экономит своё время, читая не весь код коллег, а только наиболее подозрительные места.
            +10
            P.S. Просто так, не в тему. Порадовал юмор разработчиков QT. Им было нужно какое-то магическое значение указателя для внутренних нужд. Они выбрали это:
            notified.insert( (void*)dns, (void*)42 );

              –3
              Образцовый быдлокод)
                0
                Пасхальные яйца?
                +4
                Про таблицы не согласен, затем плодить лишние массивы, если можно красиво все оформить через switch? Главное, чтобы в нем условия были отсортированы, а не шли вперемешку.
                  0
                  Таблицы еще и оверхед вносят, хотя если свитч грозится быть более, чем из 10 элементов, то лучше табличку сделать, а там поиск по ключам оптимизировать, то есть упорядочить их и юзать бинарный поиск вместо линейного как в свичах.
                    0
                    Вспоминаются исходники LastFM, файл SideBarView.cpp, где длиннющий метод с множеством case-ов.
                    Ищится по:
                    //TODO Woah! This became huge. DRY!
                    //REFACTOR IMMEDIATELY!
                      0
                      И, кстати, не факт, что так будет быстрее. Если разные ветви выполняются с разной частотой, то бинарный поиск может оказаться хуже, чем если наиболее часто используемые проверить сначала, а потом всё остальное. Но тут подводный камешек, что C++ может переставить порядок по своему усмотрению.
                      0
                      Да, согласен. Часто это тоже будет хорошим вариантом. Впрочем от забытых break; это не спасает. Надо было ведь такой синтаксис придумать для switch…
                        0
                        Да, break — это неудобно. Зато иногда требуется несколько веток исполнять сразу, и отсутствие break приходит на помощь.
                      +1
                      Длинный-предлинный switch(). И, естественно, имеется забытый оператор «break».


                      Lookup table (BTW, откуда термин «табличный метод»?) я тоже люблю. Но потерянные break ИМХО больше от неумения писать на C++. Потому как эталонный switch выглядит вот так:
                      switch( condition )
                      {
                      break; case 1:
                        Foo();
                      break; case 2:
                        Bar();
                      break; case 3:
                        Baz();
                      break; default:
                        ENSURE( false );
                      }
                      


                      Ну а те кто честно ручками ставят break после каждого case и так же ручками его там зыбывают… Каждый сам кузнец своего счастья, не?
                        0
                        В книге Code Complete таблицы называются Direct Access Tables (18.2.) и Indexed Access Tables (18.3.) Общим названием для них Стив использует lookup-tables. Наверное название «табличный метод» растёт оттуда.
                          0
                          Оттуда. Из перевода книги.
                            +2
                            Вот за что люблю русские переводы :(.
                          +2
                          Не знал о таком приеме.
                          Ситуация точно такая же, как с if (0 == x) — надежно, но неестественно смотрится :)
                            0
                            Ну мы чай не философы на художественной выставке. Естественно смотрится, неестественно смотрится… Код вообще такая штука, которая не очень естественно смотрится :). Главное — чтобы задачи решались и метрики соблюдались. А что школьникам непонятно будет — так на то они и школьники. Деятельность физиков-ядерщиков или микробиологов им тоже непонятна. ИМХО, образование отдельно, промышленное применение — отдельно.
                              +2
                              Есть и другое мнение — «программы пишут не для компьютеров, а для людей» :)

                              Компилятор-то код худо-бедно сожрет, а вот если творение предыдущего программиста без поллитры не поймешь, то печально… Я как-то переписал большой кусок кода на C# только потому, что он был написан в паскалевских традициях и развивать его было мучительно. Впрочем, этот кусок от переписывания сократился раза в два и чуток ускорился.
                                0
                                Давайте будем честны друг с другом. Этот код, так же как if( literal == variable ) замечательно читается :)
                          +4
                          Надо бы упомянуть о «несправедливости». Не знаю, правда, как компилятор VisualC++, а GCC/G++ давно умеет сворачивать switch в табличку, если это целесообразно. Конечно, табличка предполагает последовательные значения — ибо просто массив адресов, без ключей. Но для большинства enum'ов — как раз тоЮ что надо.
                            0
                            Пример с «if (q && (q->getQueryName() != _T(»BooleanQuery")" не очень убедителен, так как не очень понятно, что за сущность «q», что возвращает getQueryName и нет ли в округе каких-нибудь перегруженных операторов, меняющих явно видимую логику.
                              0
                              Хотя, простите, ступил — определение getQueryName написано в начале.
                              +3
                              А вот расскажите мне, я удивляюсь и не понимаю, как с такими ошибками (функция equals явно что-то часто сравнивает, неинициализированные переменные, неправильная функция декодирования какого-то сообщения и т.д.) это вообще работает, почему?)) И ведь эта библиотека используется во множестве проектов.
                                0
                                Я сам в шоке! :)
                                Причем в постоянном. Вот представьте, добавляю новое правило, что нет смысла в сравнениях вида if (pointer != «ABCD»). Кажется, что и не найдется то ничего. Разве только лабы у студентов проверять. А оно как полезет в разных проектах из разных щелей… Волосы дыбом. Ну что-то в таком духе:
                                #define MP4_AUDIO_TRACK_TYPE    "soun"
                                #define MP4_VIDEO_TRACK_TYPE    "vide"
                                ...
                                const char* normType = MP4Track::NormalizeTrackType(type);
                                ...
                                if (normType == MP4_AUDIO_TRACK_TYPE) {
                                  if (subType != GetTrackEsdsObjectTypeId(m_pTracks[i]->GetId())) {
                                    continue;
                                  }
                                } else if (normType == MP4_VIDEO_TRACK_TYPE) {
                                  if (subType != GetTrackEsdsObjectTypeId(m_pTracks[i]->GetId())) {
                                    continue;
                                  }
                                } 
                                

                                  +1
                                  Такой код может, кстати, и иметь смысл. Если функция NormalizeTrackType описана в этом же модуле и возвращает строковый литерал, то указатели будут совпадать.
                                    +1
                                    Это очень стрёмный путь. :)
                                  +5
                                  Все очень просто. Есть часто используеме модули и ветки кода. А есть редкоиспользуемы. Видимо, это ошибки в крайне редко используемой функциональности. Или вообще в коде, который не используется но остался «со старых времен». Плюс ряд ошибок программа может «проглотить» — тоесть если, условно, алгоритм выбора оптимального кодека работает неверно — кодек все равно будет выбран. Просто не тот :).
                                  +1
                                  Прочитал про табличные функции. Не знал что это так называется, но сам давным-давно к этому пришел.
                                    +5
                                    такое ощущение, что почти все ошибки в цпп — следствие невнимательного копипаста…
                                      0
                                      Что намекает на то, что при использовании C++ приходится писать много кода, и очень велик соблазн скопировать кусочек. Если бы не микроконтроллеры, я бы уже давно перестал писать на C++ (:
                                      0
                                      Скажите, как вы натравили PVS-Studio на исходники Qt? Когда-то выходила бесплатная редакция PVS-Studio, хотел с ней такое попробовать, но не вышло, т.к. Qt генерит какие-то кривые солюшены Visual Studio для своих исходников…
                                        +5
                                        Мы развиваемся :-). И правим-правим-правим недоработки.
                                        0
                                        как-то совсем «детские» ошибки. после этого немного страшню юзать Qt
                                          0

                                          Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                          Самое читаемое