Проект Miranda NG получает приз «дикие указатели» (часть вторая)

    Miranda NG
    Продолжим рассматривать ошибки, которые удалось обнаружить в проекте Miranda NG с помощью статического анализатора кода PVS-Studio. В прошлый раз мы говорили об указателях и работе с памятью. Теперь поговорим об ошибках общего плана, которые, в основном, связаны с неаккуратностью и опечатками.

    Продолжение проверки


    Предыдущая часть обзора кода Miranda NG доступна здесь.

    Опечатки


    Начну вот с такой красивой опечатки. Рядом с клавишей '=' находится клавиша '-'. Вот что из-за этого получилось:
    CBaseTreeItem* CMsgTree::GetNextItem(....)
    {
      ....
      int Order = TreeCtrl->hItemToOrder(TreeView_GetNextItem(....));
      if (Order =- -1)
        return NULL;
      ....
    }

    Предупреждение PVS-Studio: V559 Suspicious assignment inside the condition expression of 'if' operator: Order = — - 1. NewAwaySys msgtree.cpp 677

    Естественно, здесь должно быть написано: if (Order == -1).

    А вот тут забыли звёздочку '*':
    HWND WINAPI CreateRecentComboBoxEx(....)
    {
      ....
      if (dbv.ptszVal != NULL && dbv.ptszVal != '\0') {
      ....
    }

    Предупреждение PVS-Studio: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: dbv.ptszVal != 0 && dbv.ptszVal != '\0' SimpleStatusMsg msgbox.cpp 247

    Хотели проверить что указатель ненулевой и что строка не пустая. Но забыли разыменовать указатель. Получилось, что два раза проверили указатель на равенство нулю.

    Правильный вариант:
    if (dbv.ptszVal != NULL && *dbv.ptszVal != '\0') {

    Это ошибка обнаруживается и с помощью другой диагностики: V528 It is odd that pointer to 'wchar_t' type is compared with the L'\0' value. Probably meant: *dbv.ptszVal != L'\0'. SimpleStatusMsg msgbox.cpp 247

    Это нередкая ситуация, когда на одну ошибку указывает 2, а то и 3 диагностики. Получается так, что ошибка делает код подозрительным сразу с нескольких точек зрения.

    Есть ещё несколько V528 и я предлагаю проверить соответствующий код:
    • options.cpp 759
    • exportimport.cpp 425
    • exportimport.cpp 433
    • exportimport.cpp 441

    Некий массив заголовков копируется сам в себя. Скорее всего, здесь какая-то опечатка:
    int InternetDownloadFile (char *szUrl) 
    {
      ....
      CopyMemory(nlhr.headers, nlhr.headers,
                 sizeof(NETLIBHTTPHEADER)*nlhr.headersCount);
      ....
    }

    Предупреждение PVS-Studio: V549 The first argument of 'memcpy' function is equal to the second argument. NimContact http.cpp 46

    Вот ещё одна схожая ситуация:
    TCHAR* get_response(TCHAR* dst, unsigned int dstlen, int num)
    {
      ....
      TCHAR *tmp, *src = NULL;
      ....
      src = (TCHAR*)malloc(MAX_BUFFER_LENGTH * sizeof(TCHAR));
      ....
      _tcscpy(src, src);
      ....
    }

    Предупреждение PVS-Studio: V549 The first argument of 'wcscpy' function is equal to the second argument. Spamotron utils.cpp 218

    Строка копируется сама в себя. Я подозреваю, что в качестве одного из аргументов должен был использоваться указатель 'dst'.
    #define TTBBF_ISLBUTTON      0x0040
    
    INT_PTR TTBAddButton(WPARAM wParam, LPARAM lParam)
    {
      ....
      if (!(but->dwFlags && TTBBF_ISLBUTTON) &&
          nameexists(but->name))
        return -1;
      ....
    }

    Предупреждение PVS-Studio: V560 A part of conditional expression is always true: 0x0040. TopToolBar toolbar.cpp 307

    Скорее всего, рука дернулась и вместо '&' получилось '&&'.

    И последний случай, где вместо сравнения происходит присваивание:
    #define MAX_REPLACES 15
    INT_PTR CALLBACK DlgProcCopy(....)
    {
      ....
      if (string == newString[k])
        k--;
      if (k = MAX_REPLACES) break;
      string = oldString[++k];
      i+=2;
      ....
    }

    Предупреждение PVS-Studio: V559 Suspicious assignment inside the condition expression of 'if' operator: k = 15. NimContact contactinfo.cpp 339

    Недописанный код


    INT_PTR SVC_OTRSendMessage(WPARAM wParam,LPARAM lParam){
      ....
      CCSDATA *ccs = (CCSDATA *) lParam;
      ....
      if (otr_context_get_trust(context) >= TRUST_UNVERIFIED)
        ccs->wParam;
      ....
    }

    Предупреждение PVS-Studio: V607 Ownerless expression 'ccs->wParam'. MirOTR svcs_proto.cpp 103

    Если условие выполнилось, то ничего не произойдёт. Возможно хотели присвоить переменной «ccs->wParam» какое-то значение. Аналогичное предупреждение выдаётся также здесь: bandctrlimpl.cpp 226.

    А вот недописанный цикл:
    extern "C" __declspec(dllexport) int  Load(void)
    {
      ....
      for (i = MAX_PATH; 5; i--){
      ....
    }

    Предупреждение PVS-Studio: V654 The condition '5' of loop is always true. Xfire main.cpp 1110

    С циклом что-то не так. Мне кажется забыли сравнить 'i' с числом '5'. Плюс этот цикл скопирован ещё в одно место программы: variables.cpp 194.

    Невнимательность


    int findLine(...., int *positionInOldString)
    {
      ....
        *positionInOldString ++; 
         return (linesInFile - 1);
      }
      ....
    }

    V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. NimContact namereplacing.cpp 92

    Есть большое подозрение, что хотели изменить переменную, на которую ссылается указатель 'positionInOldString'. Но получилось, что изменили сам указатель.

    Скорее всего, код следует изменить следующим образом:
    (*positionInOldString)++; 

    «Перезапись» значения


    INT_PTR TTBSetState(WPARAM wParam, LPARAM lParam)
    {
      mir_cslock lck(csButtonsHook);
    
      TopButtonInt *b = idtopos(wParam);
      if (b == NULL)
        return -1;
    
      b->bPushed = (lParam & TTBST_PUSHED) ? TRUE : FALSE;
      b->bPushed = (lParam & TTBST_RELEASED) ? FALSE : TRUE;
      b->SetBitmap();
      return 0;
    }

    Предупреждение PVS-Studio: V519 The 'b->bPushed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 358, 359. TopToolBar toolbar.cpp 359

    Странно, что в начале в переменную было помещено какое-то значение, а затем неожиданно оно перетирается другим значением.

    Ещё один пример:
    static INT_PTR CALLBACK sttOptionsDlgProc(....)
    {
      ....
      rc.left += 10;
      rc.left = prefix + width * 0;
      ....
    }

    V519 The 'rc.left' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 583, 585. Miranda hotkey_opts.cpp 585

    Конечно, записывать в одну переменную подряд 2 разных значения не всегда является ошибкой. Иногда переменную на всякий случай инициализируют нулём, а потом используют. Есть и другие корректные ситуации. Однако я выписал 14 предупреждений, которые указывают, по моему мнению, на подозрительный код: MirandaNG-519.txt.

    Иногда предупреждение V519 косвенно выявляет ситуацию, когда забыт оператор 'break':
    void OnApply()
    {
      ....
      case ACC_FBOOK:
        m_proto->m_options.IgnoreRosterGroups = TRUE;
          
      case ACC_OK:
        m_proto->m_options.IgnoreRosterGroups = TRUE;
        m_proto->m_options.UseSSL = FALSE;
        m_proto->m_options.UseTLS = TRUE;
    
      case ACC_TLS:
      case ACC_LJTALK:
      case ACC_SMS:
        m_proto->m_options.UseSSL = FALSE;
        m_proto->m_options.UseTLS = TRUE;
        break;
      ....
    }

    Предупреждение PVS-Studio: V519 The 'm_proto->m_options.IgnoreRosterGroups' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1770, 1773. Jabber jabber_opt.cpp 1773

    Одинаковые фрагменты кода


    Встречаются места, где, независимо от условия, будут выполнены одни и те же действия.
    static void Build_RTF_Header()
    {
      ....
      if (dat->dwFlags & MWF_LOG_RTL)
        AppendToBuffer(buffer, bufferEnd, bufferAlloced,
                       "{\\rtf1\\ansi\\deff0{\\fonttbl");
      else
        AppendToBuffer(buffer, bufferEnd, bufferAlloced,
                       "{\\rtf1\\ansi\\deff0{\\fonttbl");
      ....
    }

    Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. TabSRMM msglog.cpp 439

    Возможно, код писался с помощью Copy-Paste. При этом одну из строк забыли поправить.

    Есть ещё 9 таких подозрительных мест: MirandaNG-523.txt.

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

    (прошло время)

    Итак, продолжим. Copy-Paste может проявлять себя ещё вот так:
    static int RoomWndResize(...., UTILRESIZECONTROL *urc)
    {
      ....
      urc->rcItem.top = (bToolbar && !bBottomToolbar) ?
                          urc->dlgNewSize.cy - si->iSplitterY :
                          urc->dlgNewSize.cy - si->iSplitterY;
      ....
    }

    Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: urc->dlgNewSize.cy — si->iSplitterY. TabSRMM window.cpp 473

    Зачем нужен оператор "?:", если вычисляется одно и тоже выражение?

    Ещё 11 бессмысленных тернарных операторов: MirandaNG-583.txt.

    Подозрительные операции деления
    void CSkin::setupAeroSkins()
    {
      ....
      BYTE alphafactor = 255 - ((m_dwmColor & 0xff000000) >> 24);
      ....
      fr *= (alphafactor / 100 * 2.2);
      ....
    }

    Предупреждения PVS-Studio: V636 The 'alphafactor / 100' expression was implicitly casted from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. TabSRMM themes.cpp 1753

    У меня есть подозрение, что программист хотел, чтобы деление «alphafactor / 100» было не целочисленным. Сейчас получается, что, поделив переменную типа BYTE на 100, можно получить только три значения: 0, 1 и 2.

    Вероятно, нужно делить так:
    fr *= (alphafactor / 100.0 * 2.2);

    В этом же файле можно найти ещё 2 подозрительных операции деления в строке 1758 и 1763.

    WTF?


    static INT_PTR CALLBACK DlgProc_EMail(....)
    {
      case WM_COMMAND:
        switch (LOWORD(wParam)) {
          if (HIWORD(wParam) == BN_CLICKED) {
            case IDOK:
      ....
    }

    Предупреждение PVS-Studio: V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. UInfoEx ctrl_contact.cpp 188

    Что-это за строчка «if (HIWORD(wParam) == BN_CLICKED) {» перед «case IDOK»? Она никогда не получит управление. Что вообще хотел этим сказать программист?

    Ещё одно такое место есть чуть ниже (в строке 290).

    Странно отформатированный код


    С кодом, приведённом ниже, что-то не в порядке. Но непонятно, что. То ли он неудачно отформатирован, то ли не дописан.
    int ExtractURI(....)
    {
      ....
      while ( msg[i] && !_istspace(msg[i])) 
      {
        if ( IsDBCSLeadByte(msg[i] ) && msg[i+1]) i++;
        else                                               <<<---
      
        if ( _istalnum(msg[i]) || msg[i]==_T('/')) 
        {
          cpLastAlphaNum = charCount; 
          iLastAlphaNum = i;
        }
        charCount++;
        i++;
      }
      ....
    }

    Предупреждение PVS-Studio: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. LinkList linklist_fct.cpp 92

    Обратите внимание на странный 'else'.

    Встретилось вот такое:
    void CInfoPanel::renderContent(const HDC hdc)
    {
      ....
        if (m_height >= DEGRADE_THRESHOLD)
          rc.top -= 2; rc.bottom -= 2;
      ....
    }

    Предупреждение PVS-Studio: V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. TabSRMM infopanel.cpp 370

    Очень вероятно, что программист забыл написать здесь фигурные скобки. Из 'rc.bottom' всегда вычитается двойка.

    На этом страшные истории не заканчиваются. Нужно посмотреть ещё вот сюда:
    • msn_p2p.cpp 385
    • crypt_lists.cpp 13
    • crypt_lists.cpp 44
    • common.c 273
    • common.c 307

    Остановка цикла на самом интересном месте


    bool PopupSkin::load(LPCTSTR dir)
    {
      ....
      while (hFind != INVALID_HANDLE_VALUE) {
        loadSkin(ffd.cFileName);
        break;
        if (!FindNextFile(hFind, &ffd))
          break;
      }
      ....
    }

    Предупреждение PVS-Studio: V612 An unconditional 'break' within a loop. Popup skin.cpp 807

    Зачем нужен 'break' в середине цикла? Последствие неудачного рефакторинга? И к сожалению, не единственное:
    • icq_servlist.cpp 226
    • rawping.cpp 159
    • main.cpp 304
    • gfileutils.c 266

    Всегда истинные или ложные условия


    Чаще всего эта ошибка связана с проверками вида (UNSIGNED < 0) или (UNSIGNED >=0). Но бывают и более экзотические варианты. Указатель сравнивается со строкой:
    static void yahoo_process_search_connection(....)
    {
      ....
      if (cp != "\005")
      ....
    }

    Предупреждение PVS_Studio: V547 Expression 'cp != "\005"' is always true. To compare strings you should use strcmp() function. Yahoo libyahoo2.cpp 4486

    Но вернёмся к классике жанра. Приведу только один пример, а остальные предупреждения будут, как обычно, списком.
    ULONG_PTR itemData;
    
    LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
    {
      ....
      if (dis->itemData >= 0) {
      ....
    }

    Предупреждение PVS-Studio: V547 Expression 'dis->itemData >= 0' is always true. Unsigned type value is always >= 0. TabSRMM hotkeyhandler.cpp 213

    Обещанный список: MirandaNG-547.txt.

    Кто-то не знает, как работают функции strchr() и strrchr()


    #define mir_strchr(s,c) (((s)!=0)?strchr((s),(c)):0)
    #define mir_strrchr(s,c) (((s)!=0)?strrchr((s),(c)):0)
    BYTE CExImContactBase::fromIni(LPSTR& row)
    {
      ....
      if (cchBuf > 10 && (p1 = mir_strrchr(pszBuf, '*{')) &&
          (p2 = mir_strchr(p1, '}*')) && p1 + 2 < p2) {
      ....
    }

    Предупреждения PVS-Studio:
    • V575 The 'strrchr' function processes value '10875'. Inspect the second argument. UInfoEx classeximcontactbase.cpp 177
    • V575 The 'strchr' function processes value '32042'. Inspect the second argument. UInfoEx classeximcontactbase.cpp 177
    Видимо кто-то хотел найти фрагмент текста, обрамлённого символами "*{" и "}*". Но получилась какая-то глупость.

    Во-первых, функции strchr() и strrchr() ищут один символ, а не подстроку.

    Во-вторых, '*{' интерпретируется как число 10875. Функции в качестве второго аргумента ожидают значения типа 'int', но это ничего не значит. Они используют от этого аргумента только младший байт.

    И к сожалению, это не случайная, а систематическая ошибка.

    Есть 10 таких-же некорректных вызовов: MirandaNG-575.txt.

    Неопределённое поведение


    void FacebookProto::UpdateLoop(void *)
    {
      ....
      for (int i = -1; !isOffline(); i = ++i % 50)
      ....
    }

    Предупреждение PVS-Studio: V567 Undefined behavior. The 'i' variable is modified while being used twice between sequence points. Facebook connection.cpp 191

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

    Правильней и понятней будет написать так: i = (i + 1) % 50.

    Ещё одно опасное место здесь: dlg_handlers.cpp 883.

    Теперь рассмотрим более интересный пример:
    void importSettings(MCONTACT hContact, char *importstring )
    {
      ....
      char module[256] = "", setting[256] = "", *end;
      ....
      if (end = strpbrk(&importstring[i+1], "]")) {
        if ((end+1) != '\0') *end = '\0';
        strcpy(module, &importstring[i+1]);
      }
      ....
    }

    Предупреждение PVS_Studio: V694 The condition ((end + 1) != '\0') is only false if there is pointer overflow which is undefined behaviour anyway. DbEditorPP exportimport.cpp 425

    Вообще, здесь банальная опечатка. Хотели проверить, что указатель 'end' указывает на символ, стоящий перед терминальным нулём. Ошибка в том, что забыли разыменовать указатель. Должно быть написано:
    if (*(end+1) != '\0')

    А причём здесь неопределенное поведение? Сейчас мы это обсудим.

    Вообще, эта ошибка диагностируется и другой диагностикой (V528). Но мне интересно поговорить именно о неопределённом поведении. Я хочу показать, что даже когда анализатор говорит что-то невнятное, не стоит спешить, а следует подумать, что его смущает в коде.

    Итак, прибавляя к указателю 1, мы всегда получаем значение отличное от NULL. Кроме одного единственного случая: если произойдет переполнение, мы получим NULL. Но стандарт языка говорит, что это неопределённое поведение.

    Таким образом анализатор нашел условие, которое или всегда истинно, или приводит к неопределённому поведению. А это значит, что с кодом что-то не в порядке.

    Другие неправильные проверки:
    • exportimport.cpp 433
    • exportimport.cpp 441
    • openfolder.cpp 35
    • skype.cpp 473
    И последнее на тему неопределённого поведения. Поговорим о сдвигах:
    METHODDEF(boolean)
    decode_mcu_AC_refine (....)
    {
      ....
      m1 = (-1) << cinfo->Al;
      ....
    }

    Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. AdvaImg jdarith.c 460

    Плюс:
    • jdhuff.c 930
    • cipher.c 1529

    Нет виртуального деструктора


    Есть базовый класс CNetClient:
    class CNetClient
    {
    public:
      CNetClient(): Stopped(FALSE) {}
      virtual void Connect(const char* servername,const int port)=0;
      virtual void Send(const char *query)=0;
      virtual char* Recv(char *buf=NULL,int buflen=65536)=0;
      virtual void Disconnect()=0;
      virtual BOOL Connected()=0;
      virtual void SSLify()=0;
      ....
    };

    Как видите, в нём есть виртуальные функции, но нет виртуального деструктора. От него наследуются какие-то другие классы:
    class CNLClient: public CNetClient { .... };

    И последний штрих. Есть, например, вот такой класс:
    class CPop3Client
    {
      ....
     
      class CNetClient *NetClient;
      
      ~CPop3Client() {
        if (NetClient != NULL) delete NetClient;
      }
    
      ....
    };

    Предупреждения PVS-Studio: V599 The virtual destructor is not present, although the 'CNetClient' class contains virtual functions. YAMN pop3.h 23

    Думаю, последствия понятны. Вопрос про виртуальные деструкторы задают на каждом втором собеседовании.

    Аналогично, плохо дело обстоит со следующими классами:
    • CUpdProgress
    • FactoryBase
    • ContactCompareBase

    Неправильное форматирование строк


    static const char* 
    ConvertAnyTag(FITAG *tag) {
      ....
      UINT64 *pvalue = (UINT64 *)FreeImage_GetTagValue(tag);
      sprintf(format, "%ld", pvalue[0]);
      ....
    }

    Предупреждение PVS-Studio: V576 Incorrect format. Consider checking the third actual argument of the 'sprintf' function. The argument is expected to be not greater than 32-bit. AdvaImg tagconversion.cpp 202

    О том, как делать правильно, написано здесь: "Как правильно распечатать значение типа __int64, size_t и ptrdiff_t".

    Дополнительно нужно поправить эти места в коде: MirandaNG-576.txt.

    Разное


    Странное сравнение:
    #define CPN_COLOURCHANGED     1
    #define CBN_SELCHANGE       1
    INT_PTR CALLBACK DlgPopupOpts(....)
    {
      ....
      if (wNotifyCode == CPN_COLOURCHANGED) {
        ....
      }
      else if (wNotifyCode == CBN_SELCHANGE) {
        ....
      }
      ....
    }

    Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 243, 256. PluginUpdater options.cpp 243

    Неправильно используется функция ZeroMemory():
    static int ScanFolder(....)
    {
      ....
      __except (EXCEPTION_EXECUTE_HANDLER)
      {
        ZeroMemory(szMyHash, 0);
        // smth went wrong, reload a file from scratch
      }
      ....
    }

    Предупреждение PVS-Studio: V575 The 'memset' function processes '0' elements. Inspect the third argument. PluginUpdater dlgupdate.cpp 652

    Функция ничего не обнуляет, так как второй аргумент равен нулю. Ещё один такой неправильный вызов здесь: shlipc.cpp 68.

    Двойная проверка:
    LONG_PTR CALLBACK HotkeyHandlerDlgProc(....)
    {
      ....
      if (job->hContact && job->iAcksNeeded &&
          job->hContact && job->iStatus == SendQueue::SQ_INPROGRESS)
      ....
    }

    Предупреждение PVS-Studio: V501 There are identical sub-expressions 'job->hContact' to the left and to the right of the '&&' operator. TabSRMM hotkeyhandler.cpp 523

    Мне кажется, вторая проверка ' job->hContact' просто лишняя и её можно удалить. Тем не менее, предлагаю проверить это место и вот эти:
    • ekhtml_mktables.c 67
    • affixmgr.cxx 1784
    • affixmgr.cxx 1879
    • ac.c 889
    Двойное освобождение ресурса:
    static INT_PTR ServiceCreateMergedFlagIcon(....)
    {
      HRGN hrgn;
      ....
      if (hrgn!=NULL) {
        SelectClipRgn(hdc,hrgn);
        DeleteObject(hrgn);
        ....
        DeleteObject(hrgn);
      }
      ....
    }

    Предупреждение PVS-Studio: V586 The 'DeleteObject' function is called twice for deallocation of the same resource. Check lines: 264, 273. UInfoEx svc_flagsicons.cpp 273

    Что не вошло в статью


    У меня уже больше нет сил. Многое, несущественное я поленился описывать. Ну, например, подобное:
    #define MF_BYCOMMAND 0x00000000L
    void CMenuBar::updateState(const HMENU hMenu) const
    {
      ....
      ::CheckMenuItem(hMenu, ID_VIEW_SHOWAVATAR,
        MF_BYCOMMAND | dat->bShowAvatar ? MF_CHECKED : MF_UNCHECKED);
      ....
    }

    Этот код работает не так, как предполагает программист. Но, тем не менее, он работает правильно.

    Условием тернарного оператора является не (dat->bShowAvatar), а выражение (MF_BYCOMMAND | dat->bShowAvatar). Благодаря везению, константа MF_BYCOMMAND равна нулю и никак не влияет на результат.

    И вообще, я смотрел диагностики невнимательно. Я почти сразу понял, что мне без проблем хватит на большую статью и можно особенно не присматриваться.

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

    Заключение


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

    Анализ нужно выполнять регулярно, и тогда многие ошибки будут найдены на самых ранних этапах. Это существенно сократит в дальнейшем время на их поиск и устранение.

    Приглашаю незамедлительно скачать PVS-Studio и попробовать его на своём проекте.

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Miranda NG Project to Get the «Wild Pointers» Award (Part 2).

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    497.14
    Static Code Analysis for C, C++, C# and Java
    Share post

    Similar posts

    Comments 101

      +18
      На эти тоже сказали «ошибок нет»? :)
        +12
        Судя по их репозиторию пофиксили практически все замечания из предыдущей статьи и некоторые из этой. За последние три дня в коде, где встречались ошибки, был проведен рефакторинг, но даже при рефакторинге видел как добавили новый баг(themes.cpp 1227), но через время исправили. Странно, но первый коммит который был с багом был помечен как «require for review» и скорее всего должен был проверен другим мейнтенером…
        • UFO just landed and posted this here
        –5
        Но при всем этом Миранда — просто божественный клиент для своего времени. Даже лет 8 назад всякие там Qip и прочее просто смешны были, Миранда имела плагином, наверное, даже полноценную ОС, игры и текстовый редактор =) Такое вряд ли кому снилось. Но, увы, сейчас все пошли по пути упрощения и рулят Adium и прочие. Эх, Миранда!
          +25
          ОС, игры и текстовый редактор. В мессенджере. Оооок.
          Говорят, Nero и ACDSee тоже этим путём пошли, но к успеху это их почему-то не привело.
            +6
            Нет, тут несколько иная схема.

            Они ничего не встраивали, основное ядро было «только чатица» было легкое и грамотное, вроде бы оно даже icq протокол (Oscar) не умело и его надо было доставлять. А все что хочешь сверху — пожалуйста, на плагинс.миранда.орг, а там уже фантазия ничем не ограничивалась.

            А что до неро и асидиси — они в жизни не дадут отключить эту блоатварную вакханалию, это неюзабильное ПО =)
              +2
              emacs
              +2
              Я до сих пор ею пользуюсь (хотя и гораздо реже, чем когда-то).
              Могу подтвердить, что программа действительно не самая надежная и стабильная.
              Но все альтернативы тогда чем-то не устроили, а сейчас уже по инерции.
              +25
              Вот читаешь и думаешь, а как оно вообще работает с такими ошибками?!
                –14
                Мне кажется, в закрытом ПО их в сотни раз больше. Так что совершенно не стоит ужасаться странному коду :) Вот чисто по ощущениям, когда сам пишешь код, который будет читать кто-то кроме тебя, уже совершенно иначе голова работает))))
                  +22
                  С чего вы решили, что закрытый код == никто, кроме тебя, не читает???
                    +1
                    Это зависит очень сильно от кучи факторов:
                    • Один программист в компании (бесконечно частая ситуация)
                    • Компания в целом не ориентированная на качество (такое — сплошь и рядом)
                    • Компания со средним низким уровнем программистов (править и корректировать некому)
                    • Один программист по конкретно данному ЯП

                    Поэтому даже работая в огромной компании с кучей народу имеющего доступ к коду вероятность, что его кто-то прочтет — не такая уж и высокая. Кроме того, очень сильно зависит от компании — можно ли сказать John'у, что его код — ГОВНО. Или же Вася настолько ценный сотрудник, что любая критика расценивание как заявление по собственному?

                    А вот репо на GitHub добавленный еще и в свой профиль — это уже вызов, что мол смотрите на мой код. P.S. за часть моего кода на GitHub мне-таки стыдно, да, но я работают над искоренением того, за что стыдно =)
                    +11
                    Как будто в закрытом ПО кто-то пишет код, который никто не увидит. Ну если это конечно не shareware утилита, делаемая одиночным разработчиком. Есть коллеги, начальники, есть кто сопровождает код или делает review.
                    Я анализировал код не очень большого количества закрытых проектов. Open-source было больше. Тем не менее мое ощущение такое: одинаково.
                    По тому, что видел я, вывод такой: плотность ошибок в закрытом и открытом программном обеспечении приблизительно одинакова.
                      0
                      Андрей, безусловно, но я указал много случаев, когда критику слушать не будут и поэтому чисто психологически программисты ее не бояться. Я это говорю сугубо из своей практике, когда обнаруживал «перлы» программистов уже постфактум и очень сильно матерился, что в свое время дал свободу и не лез со своей критикой. А открытый проект — это challenge и твое лицо. Если про курсовую написанную копипастом забудут (и слава великому Столлману!), то даже средне-популярный открытый проект написанный с использованием худших практик программирования выпилить из Интернета и памяти людей — почти нереальная задача :)
                  • UFO just landed and posted this here
                      +2
                      Сейчас ковыряюсь со статическим анализом опенсорсных Java-проектов. Бывает ещё:
                      5. Поведение рабочее, хотя и не такое, как имел в виду программист. Например, программист подразумевал в определённом случае к поисковому запросу автоматом подписывать слева звёздочку, но реализовал это неверно. В результате в этом случае ищется подстрока только с начала, а не с произвольного места. Пользователь может и не подумать, что это баг.
                      6. Очень много ошибок в путях обработки ошибки, потому что они редко тестируются. То есть, если всё хорошо, то будет всё хорошо, но если что-то упало, то, например, мы увидим не внятное исключение в логе или красивое сообщение об ошибке, а какое-нибудь совсем другое неадекватное исключение из другого места.
                      7. Попадаются ошибки, которые проявятся на некоторых системах. Например, (Java):
                      void processFilePath(String filePath) {
                        String[] components = filePath.split(File.pathSeparator);
                        ...
                      }
                      

                      Если программу запускают только на Unix-системах, всё будет работать. Под виндой же вы словите PatternSyntaxException, потому что тут разделитель — обратный слэш, который имеет особый смысл в регулярном выражении, а аргумент split — регулярное выражение.

                      Или вот менее тривиальный пример:
                      static Map<String, Integer> protocolPriority = new HashMap<>();
                      static {
                          protocolPriority.put("icq", 1);
                          protocolPriority.put("jabber", 2);
                      }
                      
                      public static Integer getPriority(String protocol) {
                          return protocolPriority.get(protocol.toLowerCase());
                      }
                      
                      public static void main(String... args) {
                          System.out.println(getPriority("ICQ"));
                      }
                      

                      Программа распечатает 1 в большинстве стран, но в Турции напечатает null (можете запустить с -Duser.language=tr). Думаю, туркам вообще несладко живётся — куча софта у них глючить должна.
                        0
                        А можете пояснить особенность поведения для Турции?
                          0
                          Насколько я помню, "I".toLowerCase() — совсем не "i"
                            0
                            У них есть i с точкой и без точки (пары İi и Iı).
                              0
                              У них есть i с точкой и без точки (пары İi и Iı).
                              В предпросмотре у меня выглядело, как положено… Шрифты, однако.
                              Скрытый текст

                                0
                                Спасибо. Век учись, как говорится.
                      +2
                      А под другие языки делать анализатор не хотите? В faq не нашел)
                        +10
                        Не хотим. Нами даже рынок C++ ещё не освоен.
                        Плюс С++ просто шикарное поле деятельности для таких инструментов. :)
                          –1
                          А жаль. Кмк ту же Java намного проще анализиовать — в нй ведь почти нет возможностей для разных хаков…
                            +9
                            А Java не нужно так анализировать просто. Там 2/3 этих ошибок просто не сделать.
                              –1
                              С java точно такая же история, в одном нашем проекте 10500 классов, во втором 4500 классов (суммарно 1,2 млн строк кода на java, на всех языках вместе 3,7 млн. строк по версии программы cloc). Я тоже был когда-то под впечатлением и ожидал серебряной пули и пробежался несколькими популярными анализаторами. Основная масса предупреждений — ложная либо практически бесполезная(always true, always false, дублирующиеся условия, копипаста и тп), их количество — огромно, времени на разбор руками надо очень-очень много, при этом риск внести новую ошибку(нет, не банальную опечатку) при исправлении старого когда тоже существует, код писался десятками людей и логика не всегда очевидна. При этом приоритетные баги остаются необнаруженными. В итоге от затеи я отказался.
                                0
                                А вы проверяли код уже протестированный или сразу из под программиста?
                                  0
                                  Проверялся весь проект сразу, но проблема в том, что быстро и безопасно этого не сделать. Лучший вариант это проверка перед коммитом или во время редактирования, примерно как это реализовано в идее. Надо еще понимать, что если приложение стабильно работает, от статического анализа толку будет не очень много, скорее всего все что вы исправите на самом деле ни на что не влияло, т.к. функционал оттестирован по пользовательским сценариям.
                                  Для этого же приложения заказывали платный код-ревью, и насколько я понял он свелся к прогону статическим анализатором и заглядыванием в пару классов, по результатам все ок.
                                    +1
                                    Статический анализатор как раз нужно использовать сразу в инкрементально-интерактивном режиме. К примеру, FindBugs в Java можно настроить, чтобы выполнялся автоматически после сохранения файла. Вы допустили глупую ошибку, анализатор вам сразу её подчеркнул. Вот вы и сэкономили пару минут на цикле «запустить, нажать на нужные кнопки в UI, убедиться, что ничего не работает, в отладчике посмотреть пару переменных, найти ошибку». Если вы запустите статический анализатор после, он вам конечно меньше найдёт, но время вы уже потеряли.

                                    При этом в коде, который десять лет как написан и протестирован, и им пользуются сотни тысяч и миллионы пользователей, всё равно статическим анализом находятся ошибки из серии «а как это вообще работает?»
                                      +1
                                      Я так понял, что код протестированный руками и отревьювленный глазами особого смысла проверять нет. Если что-то найдете, это будет неважное, мертвый код и т.д.

                                      Имеет смысл проверять до ручного тестирования инкрементально. То есть начать с того, что поместить в игнор все что существует сейчас, а потом вставить его где-то между компилятором и тестером, лучше ближе к компилятору.
                                +4
                                Для Java есть FindBugs.
                                Плюс нормальные среды разработки умеют сами много чего находить.
                                  0
                                  В этом плане полезно поставить sonar и фигачить в него исходниками (например, через хук репозитория, maven, gradle etc). Можно локально, можно на сервере. Раньше там был findbugs, pmd, checkstyle и т. п. Какое сейчас состояние по плагинам — не знаю.
                              0
                              Если интересует Perl, то есть Perl Critic (а также крутая книга — Perl Best Practics). Там очень крутой анализатор, он по части стиля и приемов кодирования, но в случае Perl — это решает половину возможных проблем просто запретом ряда фич =)
                                0
                                Вот за что его нежно «люблю», так это за ругань на unless… (я про Critic, само собой, а не про перл)
                                  0
                                  У него регулируется уровень :)
                                    0
                                    Дык я знаю, просто в какой-то момент Critic превращает Perl в C :-)
                              +4
                              The first argument of 'memcpy' function is equal to the second argument.
                              Это вообще UB согласно стандарту, потому что он требует, чтобы источник и приемник у memcpy не пересекались. Хотя на практике будет работать, конечно.
                                –1
                                Хотя на практике будет работать, конечно.

                                Закон Мёрфи тут тоже действует: lwn.net/Articles/414467/
                                  +1
                                  Случай одинаковых аргументов тем изменением затронут не был…
                                    0
                                    Два одинаковых аргумента тем более указывают на пересекающиеся области памяти.
                                      +1
                                      Да, но ошибка при этом «чисто случайно» не возникает
                                    0
                                    Удалено
                                  +3
                                  Вообще, странно, что такие ошибки в коде присутствуют. Даже встроенные в IDE анализаторы кода часть таких ошибок детектируют и выводят предупреждения. Видать, совсем глаза закрывают на предупреждения — знакомая ситуация, признаться. Компилируется — вот главный показатель! :)
                                  +15
                                  V547 Expression 'tszValue.GetBuffer()[0] == TCHAR(9835)' is always false. The value range of char type: [-128, 127]. VKontakte vk_thread.cpp 354
                                  V547 Expression 'StatusText.GetBuffer()[0] != TCHAR(9835)' is always true. The value range of char type: [-128, 127]. VKontakte vk_thread.cpp 1055

                                  Я не стану комментировать никакие другие детекты из этой статьи, но этот код написан мной, так что не могу пройти мимо. Как мне кажется, здесь речь идет, как минимум о неточном, а как максимум – о ложном срабатывании. В обоих случаях сравниваются TCHAR значения. В моем случае проект однозначно не ANSI и никогда, я подчеркиваю, никогда им не будет, а, следовательно, TCHAR разворачивается только в wchar_t и никогда в char. Конечно, можно порассуждать о том, что теоретически такое все-таки может случится, но в любом случае говорить о том, что «Expression TCHAR(9835)' is always true» нельзя, ведь PVS же проверяет при анализе определено _WIN32 или нет. Можно лишь говорить, что выражение может быть при определенных условиях, всегда истинным.
                                  Теперь что касается вообще. Я лично не считаю статические анализаторы бесполезными, потому вчера скачал PVS-студию и прогнал ей свой проект (VKontakte). Из 37 детектов (это L1 и L2, только GA) 2 оказались полезными, остальные, в том числе и “проверка на NULL после использования”, оказались ложными. Мой проект сравнительно небольшой, я знаю логику его работы, он не заброшен и активно развивается, потому я могу потратить полчаса на анализ PVS-студией. Заплатил бы я за нее деньги (ведь цель этих двух статей – привлечь покупателей, в первую очередь)? С таким процентом полезного выхода – однозначно нет, учитывая, что разрабатываемый проект бесплатный.
                                  Будь у меня более двух миллионов строк кода, причем, в значительной массе своей, не моего кода, кода в проектах, которые заброшены/редко используются и взяты в общий транк часто “за компанию” и адаптированы в NG на скорую руку, при том, что большинство таких багов сработают в экзотичных условиях, я бы, возможно, как это сделал Хазан, тоже сравнил бы такую помощь с бесплатной футболкой, за которой надо ехать в соседний город.
                                  Тем не менее, ругать и опускать PVS студию мне не за что, поскольку моему проекту она пользу принесла. Считаю необходимым и правильным поблагодарить Вас за проделанную работу. Так же, пользуясь случаем, хочу поблагодарить и Ivan_83 за его исправления. В любом случае, считаю, что эти две статьи принесли проекту Miranda NG пользу.
                                    0
                                    Теперь что касается вообще. Я лично не считаю статические анализаторы бесполезными, потому вчера скачал PVS-студию и прогнал ей свой проект (VKontakte). Из 37 детектов (это L1 и L2, только GA) 2 оказались полезными, остальные, в том числе и “проверка на NULL после использования”, оказались ложными. Мой проект сравнительно небольшой, я знаю логику его работы, он не заброшен и активно развивается, потому я могу потратить полчаса на анализ PVS-студией. Заплатил бы я за нее деньги (ведь цель этих двух статей – привлечь покупателей, в первую очередь)? С таким процентом полезного выхода – однозначно нет, учитывая, что разрабатываемый проект бесплатный.


                                    Прокомментирую эту часть. Хоть лично Вы это 100% понимаете и написали ниже, для других хочу донести и нашу позицию. Однозначно статический анализ полезен прежде всего на средних и больших проектах. Если код проекта может просмотреть за вечер один человек, то так и надо делать (время от времени). И если разрабатывается проект одним человеком — тоже ему никто не предлагает тратить кучу денег на статический анализ. Может ли быть польза для «малышей» (маленьких проектов) от статического анализа? Да может. И обучение, и поиск ошибок все-же. Стоит ли тратить «малышам» (маленьким проектам) деньги на статический анализ? Скорее всего нет.

                                    И спасибо за подробный комментарий и оценку нашей работы.
                                      –9
                                      ОК ей, наш проект большой:

                                      6 миллионов строк. Возраст кода до 8 лет. Авторов 3/4 кода давно в проекте нет. Документация вида «код — лучшая документация». В проекте огромное число вот таких вот ошибок зачастую в OutOfCoverage. Куча макросов из соображений «надоело писать одинаковый код». Некоторые файлы тысяч на 80 строк. В принципе не существует людей которые бы знали весь код. Никто не сможет гарантировать как одно повлияет на другое. Ну и прочие прелести проприетарного промышленного кода с огромным багажом.

                                      Но при этом качество работы продукта заказчика устраивает. Более того 24/7.

                                      Поробовал PVS — упал. Убрав особо-изощренные файлы PVS отработал. Среди найденного количества ошибок (оно в демке вроде ограниченно было сотней) самое страшное что нашлось — присвоение в if в том месте которое не используется вообще. Остальное разный мусор из серии депрекатов.
                                      Вывод: да что-то откровенное оно нашло, но только не в полезном коде. Тратить время разработчиков на разбор ложноположительного — глупо, а его там откровенно много. Риск исправлением внести новый дефект — очень высокий. Зачастую «это жжж тут не спроста».

                                      Качество продукта устраивает, при этом существуют известные дефекты, которые имеют конкретный сценарий.

                                      Предположим я куплю продукт и он найдет мне 1000 косяков, мы потратим X человекочасов разработчиков на исправление. Потом X*K человекочасов тестеров на полную перепроверку ВСЕГО функционала ибо эти косяки будут размазаны ровным слоем по всему коду. Итого: потраченные деньги, потраченное время, сомнительная выгода по качеству продукта.

                                      Вопрос зачем мне платить деньги за это?
                                        +13
                                        Вы неправильно используете PVS, пусть и гипотетически. Требуется: взять ревизию, скажем, месячной давности, проверить ее, занести все найденные ошибки в список ложноположительных. Возможно, выключить при этом какие-то диагностики. Потом взять свежий код, и начать регулярно проверять его. В таком режиме статический анализатор начнет экономить человеко-часы, а не тратить.

                                        Статические анализаторы — для поиска свежих ошибок, а не залежавшихся.
                                      +10
                                      Не могу пройти мимо TCHAR… Зачем вообще его использовать, если проект никогда не уйдет с Юникода? Особенно в таких местах, где кроме как с Юникодом работать невозможно. Было бы куда правильнее использовать тип CMStringW вместо CMString, и сравнивать с числом без приведения к TCHAR.
                                        +1
                                        Тут дело скорее вкуса и привычки. Ну и есть еще такая вещь, как доставшийся по наследству код, в котором для юникода уже повсеместно используется TCHAR/ptrT/CMString.
                                          0
                                          Допустим, этот пример был бы написан правильно:
                                          #ifdef _UNICODE
                                          #define MUSIC_CHAR 9835
                                          #else
                                          #define MUSIC_CHAR 0x0E
                                          #endif
                                          // ...
                                          if (tszValue.GetBuffer()[0] == TCHAR(MUSIC_CHAR))

                                          для PVS-студии что нибудь изменилось бы?
                                            +1
                                            Нет. Это недоработка в PVS-Studio. Со временем исправится. :)
                                          +13
                                          Мне кажется, что вы не совсем правы, утверждая, что «Из 37 детектов (это L1 и L2, только GA) 2 оказались полезными».
                                          Польза детектов может быть не только в обнаружении ошибок, но и в обнаружении мест, которые, не смотря на то, что работают идеально правильно, и более того, так и задумывались, могут на самом деле быть «плохими».

                                          Проект ведь OpenSource. Любой человек может придти и начать работать с кодом. Вот что я подумаю, когда увижу TCHAR? Вряд-ли именно то, что «проект однозначно не ANSI и никогда, я подчеркиваю, никогда им не будет». Как раз таки обратное, TCHAR ведь. И я уже введён в заблуждение(как и анализатор). И возможно, из-за своих заблуждений(ведь там ещё было, по вашим же словам 35 «бесполезных» срабатываний) я внесу баг, который попротит много нервов как разработчикам, так и пользователям.

                                          Пусть анализатор и не показывает реальных ошибок в тех местах, но он как минимум показывает множество не самых хороших решений, которые автору кода, возможно, казались удачными. И исправлениепереписывание которых поможет новым(а может и старым, но работавшим над другой частью проекта) разработчикам допускать меньше ошибок.
                                            +5
                                            Другими словами PVS-Studio выдает не список ошибок, а список мест, которые стоит посмотреть разработчику и возможно что-то улучшить/поправить там.

                                            Да, конечно лучше бы анализатор выдавал список ошибок. Тогда бы его можно было продавать в десятки раз дороже :-). К сожалению, это невозможно.
                                            +1
                                            Да, это ложное срабатывание. Видимо анализатор подзапутался с шаблонными типом переменной tszValue. Неприятно, но не страшно. Если такое возникает у пользователя или человека, желающего стать пользователем, мы устраняем такие ошибки. Правки, доработки, и поддержка являются составляющей частью приобретённой лицензии. Многим нравится наша поддержка и они вновь и вновь продлевают лицензию. Кстати, в анализаторе уже достаточно много специфичных настроек для различных проектов. Они скрыты от глаз, так как предназначаются для конкретных проектов клиентов.

                                            Хочу немного поговорить на тему полезного выхода (процента ложных срабатываний).
                                            Я не планирую оправдывать инструмент. Ложные срабатывания — это зло, и мы боремся с ними как можем. Но следует учитывать, что есть несколько факторов, узнав про которые люди начинают по-другому смотреть на этот вопрос. Напомню их для читателей.
                                            • Основная ценность статического анализа в его регулярном применении. Аналогия — предупреждения компилятора. Первый запуск статического анализа для работающего проекта всегда несколько бестолков. Большинство ошибок, который мог бы при разработке проекта найти статический анализ уже исправлены более дорогими методами тестирования. Если бы это было не так, проект бы не работал. Подробнее на эту тему есть хорошая заметка: "Лев Толстой и статический анализ кода".
                                            • Небольшие проекты дают немного искажённое представление о пользе статического анализа. Дело в том, что в них ниже плотность ошибок. Proof: "Ощущения, которые подтвердились числами".
                                            • Статический анализатор требует хотя-бы минимальной настройки. Мы стремимся, чтобы даже первый запуск был полезен (вывод не был забит ложными срабатываниям). Но далеко не всегда так получается. Поэтому в PVS-Studio есть масса инструментов для борьбы с ложными срабатываниями. И часто можно очень быстро отсеять большинство из них. Статья на эту тему: "Работа с ложными срабатываниями в PVS-Studio и CppCat".

                                            0
                                            Поспорил бы насчёт виртуальных деструкторов, кстати, не всегда они необходимы.

                                            Так, например, std::shared_ptr отлично запоминает, с каким конкретным типом объекта он был создан, и вызывает нужный deleter для него без всяких виртуальных деструкторов у самого класса (там всё это в type erasure внутри реализации shared_ptr запихивается). Иными словами, с невиртуальными деструкторами у Base и Derived здесь всё будет правильно:
                                            std::shared_ptr<Base> basePtr = std::make_shared<Derived> ();
                                            


                                            Но это так, в порядке придирок. И если писать на современном C++11 или использовать Boost (там ЕМНИП та же семантика). Другое дело, что, судя по одному вчерашнему обсуждению в конференции miranda-ng, там этого не будет, скорее всего, никогда.
                                              0
                                              Запоминает в случае использования std::make_shared, верно?
                                                0
                                                В случае прямого дёрганья конструктора shared_ptr тоже запоминает.
                                                  0
                                                  Главное при этом указатель правильного типа в конструктор (конструктор — шаблонный) передать, вот так будет работать:
                                                  std::shared_ptr<Base> a(new Derived);
                                                  а вот так уже, разумеется, нет:
                                                  Base* b = new Derived;
                                                  std::shared_ptr<Base> c(b);
                                                  
                                                    0
                                                    Да, естественно. Магии не бывает.

                                                    Ну и писать голые new вообще не очень здорово, исключения там всякие, утечки.

                                                    Хотя, конечно, это всё для идеального мира. В моём проекте с Qt и её взглядами на иерархии объектов, управление памятью и так далее, к сожалению, так не получится.
                                                      0
                                                      Почему?
                                                        +1
                                                        Потому что кушает условный, не знаю, QAbstractItemView::setItemDelegate() или QLayout::addWidget() сырой указатель на объект, владение которым ему передастся, и всё тут, никаких умных указателей.
                                                          0
                                                          Да, ясно. Впрочем, тут тоже есть где использовать умные указатели — но выгода от них не настолько большая.
                                                      –1
                                                      Эээ… А как это работает? Откуда в первом случае берётся информация о типе? Мне казалось, эти две конструкции эквивалентны.
                                                        +2
                                                        Из шаблонного конструктора:
                                                        template <T> class shared_ptr {
                                                          //...
                                                          template <Q> shared_ptr(Q* ptr);
                                                        };
                                                        

                                                        Примерно так оно выглядит внутри. Соответственно, несмотря на то, что создается умный указатель на T — дестурктор запоминается для типа Q.
                                                          0
                                                          У std::shared_ptr конструктор шаблонный и он создает deleter на основе типа переданного объекта, а не на основе типа самого умного указателя, и только после этого происходит конвертация полученного указателя в тип умного указтеля.
                                                    +1
                                                    Кстати, есть ещё подобная красивая «магия» при биндинге временной переменной на ссылку:
                                                    const Base& base_ref = Derived(); // Или любая функция, возвращающая Derived по значению
                                                    
                                                    По окончании времени жизни ссылки будет вызван деструктор Derived, а не Base.
                                                      0
                                                      Да, это вполне разумно. Правда, не понимаю, зачем так писать, если честно :) По крайней мере, сходу не получилось сценария, когда в этой точке необходимо типу быть const Base&, а не Derived.
                                                        0
                                                        Все просто: использование наиболее общего класса из возможных считается хорошей практикой.
                                                          +2
                                                          Не уверен, что это настолько уж имеет смысл в контексте локальных переменных. Уж лучше писать const auto& тогда, ИМХО.
                                                          0
                                                          Я тоже не знаю, где это можно применить. Насколько я понимаю, это побочный эффект того, что правила инициализации для ссылок-локальных переменных и ссылок-параметров функций одинаковы. А, скажем, для функции вида void f(const Base&) вызов f( Derived() ) вполне осмысленен.
                                                        0
                                                        Анализатор PVS-Studio (в отличии, например, от C4265 в Visual C++) ругается не на все классы без виртуального конструктора. Он негодует только в том случае, если явно вызывается оператор delete для базового класса. Подробнее: Описание V599.
                                                          0
                                                          То есть на вот такой код он ругаться не будет?
                                                          // У класса Base нет виртуального деструктора
                                                          Base *base = new Derived();
                                                          std::shared_ptr<Base> ptr = base;
                                                          
                                                            +3
                                                            К сожалению нет. Кстати, спасибо за пример. Выписал.
                                                              0
                                                              А после вашего исправления на
                                                              Base *base = new Base();
                                                              std::shared_ptr<Base> ptr = base;
                                                              

                                                              оно ругаться не начнёт?

                                                              Отвечая заодно на ваше сообщение чуть выше: я не очень понял, что имеется ввиду под вызовом оператора. Только лишь руками в пользовательском коде или также и в любой библиотечной функции? Ведь, конце концов, shared_ptr рано или поздно тоже delete вызовет (если делитер не кастомный), так что, формально, оператор delete для базового класса и в этом случае вызовется. А логика подсказывает, что анализатор должен ругаться при обнаружении любого вызова delete.
                                                                0
                                                                Начнёт. Не хочу вдаваться в подробности. Главное не забывать, что это статический, а не динамический анализ. У него меньше знаний о том, что это за указатель, откуда он пришёл и т.д. www.viva64.com/ru/b/0248/
                                                                  0
                                                                  Да, если быть честным, оно (пока) и не особо нужно. Сомневаюсь, что в реальном мире достаточно много кода, для которого такая диагностика была бы ошибочной и лишней.
                                                      • UFO just landed and posted this here
                                                          0
                                                          Тут проблема в ++i, вместо него должно быть (i + 1), и выражение обретает нормальный вид, не считая, конечно, явно впечатанной константы 50.
                                                            +1
                                                            Чего-то я поспешил, согласно новому стандарту (C++11) здесь нету неопределенного поведения, вот если ++i заменить на i++, то появится. Старый стандарт (С++03) имел более строгие правила, согласно ему это UB.
                                                              +1
                                                              Так и было предупреждение для старого стандарта (видно из наличия термина «sequence points»).
                                                                +1
                                                                Я сейчас ощутил, что анализатор PVS-Studio становится стареньким. :) В том смысле, что он поддерживает ещё VS2005 и так далее. А значит должен для разных версий языка иногда выдавать разные предупреждения или не выдавать. Как в этом примере. Конкретно, в этом случае думаю ничего не надо изменять в анализаторе. Пусть лучше ругается. Все равно код не очень хорош и его стоит поправить.
                                                                0
                                                                Выражение while — не самый лучший выход, потому что изменить переменную i внизу цикла легко позабыть. Конечно, это совсем не значит, что нужно всякий раз писать что-то вроде for (int i=0, j=1; i<n; i=j-i, j=1-j) — но операция инкремента по модулю, как мне видится, все же довольно простая для понимания.
                                                                –1
                                                                Лично я за 4 года использования clang понял, что статический анализ кода на сях/плюсах мне не нужен. Я не говорю, что он не нужен никому, я говорю, что он не нужен лично мне. Я пишу на C/C++ уже более 10 лет, причем пишу на нем в основном только low-level структуры данных/алгоритмы, драйвера какие-нибудь и т.д. Учитывая специфику решаемых задач, почти всё время уходит на мыслительный процесс, а не на написание кода. Ошибки в коде конечно же бывают, но для того, чтобы их найти, нужен глубокий человеческий анализ и понимание алгоритма. Статические анализаторы находят только совсем очевидные вещи, учитывая, с какой скоростью я пишу код и как обдумываю каждую строчку, у меня таких ошибок просто не возникает. Статический анализ полезен только в проектах, где пишут мало осмысленный код с огромной скоростью (в основном это десктопные приложения на плюсах, вроде той же миранды).
                                                                Приглашаю незамедлительно скачать PVS-Studio и попробовать его на своём проекте.

                                                                У вас на сайте вообще нигде (кроме страницы скачивания) не написано, что это всего лишь плагин к Visual Studio, хотя используется громкое название «статический анализатор PVS-Studio». Нужена как минимум standalone-версия, чтобы можно было по-быстрому проверить проект и не ставить ради одной проверки Visual Studio. А ещё неплохо бы иметь не только кросс-IDE версию, но и кросс-платформенную, чтобы под Linux и OSX можно было проверять.
                                                                    0
                                                                    Ок, standalone есть. Теперь бы заставить эту standalone-версию работать под OSX и Linux, было бы замечательно.
                                                                      0
                                                                      Ну, чтобы называться статическим анализатором, standalone версии под винду вполне достаточно :)
                                                                        0
                                                                        Об этом можно и написать на сайте — так и так, есть плагин к VS и standalone, etc… А то сейчас страница скачивания вводит в заблуждение. Когда я последний раз пробовал PVS, этого не было.
                                                                    0
                                                                    > у меня таких ошибок просто не возникает.
                                                                    Даже гроссмейстеры могут получить детский мат.
                                                                      +1
                                                                      Могут, но не нужно забывать, что описываемый анализатор платный и его покупка должна быть целесообразна. В моем случае это не целесообразно. А возможность такой ошибки в своем коде я не отрицаю. Возможно судить обо всех статических анализаторах по clang неправильно, но тем не менее, он то бесплатный (и встроен в компилятор), и я могу его позволить как «дополнительный» уровень защиты от всякого рода ошибок, даже если он мне не помогает. Другое дело PVS Studio, даже если он нашел бы ошибку там, где не справился clang, много ли было бы таких случаев? Вряд ли. Потому что почти все ошибки связаны с логикой работы, а такие ошибки не поддаются статическому анализу.
                                                                        +1
                                                                        Гляньте вот эту стаьтью: habrahabr.ru/company/pvs-studio/blog/233179/
                                                                        PVS Studio и clang находят совершенно разные классы ошибок. В этом блоге неоднократно были отчеты о проверках опенсорсных проектов, проверяемых регулярно другими анализаторами — но PVS все равно что-то да находила.
                                                                          0
                                                                          Говорю же, ошибки, которые обнаруживаются статическим анализом — это очень маленькое подмножество возможных ошибок. Не важно, что конкретно ищет анализатор, даже идеального анализатора будет мало чтобы окупить его стоимость. Ни компилятор, ни анализатор не знают, что за алгоритм вы пишите и как он должен себя вести. А вот хорошие юнит-тесты очень даже помогают.
                                                                            +2
                                                                            Мне кажется основное преимущество — это очень раннее обнаружение ошибок. Допустим вы сделали какую-то глупую опечатку (скобку не там поставили, например), а она не привела к ошибке компиляции. Конечно же у вас ничего не заработает, и вы с помощью отладки или внимательно прочитав несколько раз код эту опечатку в итоге найдете. Но если анализатор ткнет вас в неё носом прям в момент компиляции, вы сэкономите время на поиск такой ерунды.
                                                                              0
                                                                              В идеале анализатор должен на лету подчёркивать опечатки и ошибки, как это делает R#
                                                                                0
                                                                                Время-то он конечно экономит, вот только парадокс в том, что сам по себе статический анализ замедляет сборку (а в случае PVS довольно сильно) и не все запускают его прям после каждой правки в коде :) Каждый должен сам для себя решить, если такое происходит постоянно, то может быть анализатор и стоит покупки.
                                                                                0
                                                                                Юнит тесты тоже не серебряная пуля. И их тоже стоит проверять ;). Разные методы хорошо дополняют друг друга. Заметка: "Как статический анализ дополняет TDD".
                                                                              0
                                                                              Потому что почти все ошибки связаны с логикой работы, а такие ошибки не поддаются статическому анализу.

                                                                              Да, но лучше найти хоть что-то, чем ничего. Это что-то может ещё как пить кровь. Хороший пример: habrahabr.ru/post/198836/
                                                                            –1
                                                                            Лично я за 4 года использования clang понял, что статический анализ кода на сях/плюсах мне не нужен.

                                                                            А я понял, что нужен. Хотя я бы не сказал, что мы во всю гов*кодим :). Заметка на эту тему: "Проверка PVS-Studio с помощью Clang. С тех пор, мы используем для регулярного анализа не только PVS-Studio, но и Clang (запускается по ночам вместе с другими разными проверками).

                                                                            Я бы не был столь критичен в отрицании простых ошибок. Вот я их допускаю и мне не стыдно про это сказать. Почему — потому, что их допускают все. Очень рекомендую к прочтению: Мифы о статическом анализе. Миф второй – профессиональные разработчики не допускают глупых ошибок.

                                                                            Статический анализ полезен только в проектах, где пишут мало осмысленный код с огромной скоростью (в основном это десктопные приложения на плюсах, вроде той же миранды).
                                                                            А вот кстати и список этих бредовых проектов: www.viva64.com/ru/a/0084/
                                                                              –1
                                                                              Попробуйте: PVS-Studio для Linux.
                                                                              –5
                                                                              Ошибка в том, что забыли разменивать указатель
                                                                              Вам не хватает статического анализа смысла предложений которые вы пишете :)
                                                                              Разыменовать, разыменование!
                                                                                0
                                                                                Разрабатываем новые диагностики. В связи с этим попалось вот такое новое:

                                                                                V718 Suspicious type conversion: HRESULT -> BOOL. Clist_modern modern_xptheme.cpp 180

                                                                                #define THEMEAPI          EXTERN_C DECLSPEC_IMPORT HRESULT STDAPICALLTYPE
                                                                                
                                                                                THEMEAPI
                                                                                EnableThemeDialogTexture(
                                                                                    __in HWND hwnd,
                                                                                    __in DWORD dwFlags
                                                                                    );
                                                                                
                                                                                BOOL xpt_EnableThemeDialogTexture(HWND hwnd, DWORD flags)
                                                                                {
                                                                                	BOOL res = FALSE;
                                                                                	xptlock();
                                                                                	res = EnableThemeDialogTexture(hwnd, flags);
                                                                                	xptunlock();
                                                                                	return res;
                                                                                }
                                                                                

                                                                                BOOL и HRESULT очень разные сущности. Их значения надо проверять по разному. Следовательно, смешивать их нельзя. Дело в том, что:

                                                                                #define S_OK   ((HRESULT)0L)
                                                                                #define S_FALSE  ((HRESULT)1L)
                                                                                
                                                                                #define FALSE  0
                                                                                #define TRUE   1
                                                                                


                                                                                Ещё из странного:

                                                                                V717 It is strange to cast object of base class 'avatarCacheEntry' to derived class 'CacheNode'. AVS cache.cpp 83

                                                                                
                                                                                typedef struct avatarCacheEntry { .... };
                                                                                
                                                                                struct CacheNode : public avatarCacheEntry, public MZeroedObject
                                                                                {
                                                                                	CacheNode();
                                                                                	~CacheNode();
                                                                                
                                                                                	BOOL   loaded;
                                                                                	DWORD  dwFlags;
                                                                                	int    pa_format;
                                                                                
                                                                                	void   wipeInfo();
                                                                                };
                                                                                
                                                                                avatarCacheEntry tmp;
                                                                                ....
                                                                                CacheNode *cc = arCache.find((CacheNode*)&tmp);
                                                                                


                                                                                Аналогично:

                                                                                AVS cache.cpp 173

                                                                                AVS cache.cpp 199

                                                                                Only users with full accounts can post comments. Log in, please.