Подсчитаем баги в калькуляторе Windows


    На днях компания Microsoft открыла исходный код калькулятора. Это приложение входило во все версии операционной системы Windows. Исходный код разных проектов Microsoft достаточно часто открывался за последние годы, но новость о калькуляторе в первый же день просочилась даже в нетехнологические средства массовой информации. Что ж, это популярная, но очень маленькая программа на языке C++. Тем не менее, статический анализ кода с помощью PVS-Studio выявил подозрительные места в проекте.

    Введение


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

    Общественность, например, уже обратила внимание на такую функцию:
    void TraceLogger::LogInvalidInputPasted(....)
    {
      if (!GetTraceLoggingProviderEnabled()) return;

      LoggingFields fields{};
      fields.AddString(L"Mode", NavCategory::GetFriendlyName(mode)->Data());
      fields.AddString(L"Reason", reason);
      fields.AddString(L"PastedExpression", pastedExpression);
      fields.AddString(L"ProgrammerNumberBase", GetProgrammerType(...).c_str());
      fields.AddString(L"BitLengthType", GetProgrammerType(bitLengthType).c_str());
      LogTelemetryEvent(EVENT_NAME_INVALID_INPUT_PASTED, fields);
    }

    которая логирует текст из буфера обмена и, возможно, отправляет его на серверы Microsoft. Но эта заметка не об этом. Хотя подозрительных примеров кода будет много.

    Мы проверили исходный код калькулятора с помощью статического анализатора PVS-Studio. Так как код написан на нестандартном C++, многие постоянные читатели блога анализатора усомнились в возможности анализа, но это оказалось возможным. C++/CLI и C++/CX поддерживаются анализатором. Некоторые диагностики выдали ложные предупреждения из-за этого, но ничего критичного не произошло, что помешало бы воспользоваться этим инструментом.

    Возможно, вы пропустили новости и о других возможностях PVS-Studio, поэтому хочу напомнить, что кроме проектов на языках C и C++, можно проанализировать код и на языках C# и Java.

    Про неправильное сравнение строк


    V547 Expression 'm_resolvedName == L«en-US»' is always false. To compare strings you should use wcscmp() function. Calculator LocalizationSettings.h 180
    wchar_t m_resolvedName[LOCALE_NAME_MAX_LENGTH];
    
    Platform::String^ GetEnglishValueFromLocalizedDigits(....) const
    {
      if (m_resolvedName == L"en-US")
      {
        return ref new Platform::String(localizedString.c_str());
      }
      ....
    }

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

    Дело в том, что здесь неправильно сравниваются строки. Получилось сравнение указателей вместо значений строк. Сравнивается адрес массива символов с адресом строкового литерала. Указатели всегда неравны, поэтому условие всегда ложно. Для правильного сравнения строк следует использовать, например, функцию wcscmp.

    Кстати, пока я пишу эту статью, в заголовочном файле массив символов m_resolvedName превратился в полноценную строку типа std::wstring. И теперь сравнение работает правильно. К моменту, когда вы будете читать эту статью, скорее всего, многие другие ошибки тоже будут исправлены благодаря энтузиастам и таким исследованиям, как это.

    Утечка памяти в нативном коде


    V773 The function was exited without releasing the 'temp' pointer. A memory leak is possible. CalcViewModel StandardCalculatorViewModel.cpp 529
    void StandardCalculatorViewModel::HandleUpdatedOperandData(Command cmdenum)
    {
      ....
      wchar_t* temp = new wchar_t[100];
      ....
      if (commandIndex == 0)
      {
        delete [] temp;
        return;
      }
      ....
      length = m_selectedExpressionLastData->Length() + 1;
      if (length > 50)
      {
        return;
      }
      ....
      String^ updatedData = ref new String(temp);
      UpdateOperand(m_tokenPosition, updatedData);
      displayExpressionToken->Token = updatedData;
      IsOperandUpdatedUsingViewModel = true;
      displayExpressionToken->CommandIndex = commandIndex;
    }

    Мы видим указатель temp, ссылающийся на массив из 100 элементов, под который выделена динамическая память. К сожалению, память освобождается всего в одном месте функции, во всех остальных местах возникает утечка памяти. Она не очень большая, но это всё равно ошибка для C++ кода.

    Неуловимое исключение


    V702 Classes should always be derived from std::exception (and alike) as 'public' (no keyword was specified, so compiler defaults it to 'private'). CalcManager CalcException.h 4
    class CalcException : std::exception
    {
    public:
      CalcException(HRESULT hr)
      {
        m_hr = hr;
      }
      HRESULT GetException()
      {
        return m_hr;
      }
    private:
      HRESULT m_hr;
    };

    Анализатор обнаружил класс, унаследованный от класса std::exception через модификатор private (модификатор по умолчанию, если ничего не указано). Проблема такого кода заключается в том, что при попытке поймать общее исключение std::exception исключение типа CalcException будет пропущено. Такое поведение возникает потому, что приватное наследование исключает неявное преобразование типов.

    Пропущенный день


    V719 The switch statement does not cover all values of the 'DateUnit' enum: Day. CalcViewModel DateCalculator.cpp 279
    public enum class _Enum_is_bitflag_ DateUnit
    {
      Year = 0x01,
      Month = 0x02,
      Week = 0x04,
      Day = 0x08
    };
    
    Windows::Globalization::Calendar^ m_calendar;
    
    DateTime
    DateCalculationEngine::AdjustCalendarDate(Windows::Foundation::DateTime date,
                                              DateUnit dateUnit, int difference)
    {
      m_calendar→SetDateTime(date);
    
      switch (dateUnit)
      {
        case DateUnit::Year:
        {
          ....
          m_calendar->AddYears(difference);
          m_calendar->ChangeCalendarSystem(currentCalendarSystem);
          break;
        }
        case DateUnit::Month:
          m_calendar->AddMonths(difference);
          break;
        case DateUnit::Week:
          m_calendar->AddWeeks(difference);
          break;
      }
    
      return m_calendar->GetDateTime();
    }

    Подозрительно, что в switch не рассмотрен случай с DateUnit::Day. Из-за этого в календарь (переменная m_calendar) не добавляется значение, связанное с днём, хотя метод AddDays у календаря присутствует.

    Ещё несколько подозрительных мест с другим перечислением:
    • V719 The switch statement does not cover all values of the 'eANGLE_TYPE' enum: ANGLE_RAD. CalcManager trans.cpp 109
    • V719 The switch statement does not cover all values of the 'eANGLE_TYPE' enum: ANGLE_RAD. CalcManager trans.cpp 204
    • V719 The switch statement does not cover all values of the 'eANGLE_TYPE' enum: ANGLE_RAD. CalcManager trans.cpp 276

    Подозрительные сравнение вещественных чисел


    V550 An odd precise comparison: ratio == threshold. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. Calculator AspectRatioTrigger.cpp 80
    void AspectRatioTrigger::UpdateIsActive(Size sourceSize)
    {
      double numerator, denominator;
      ....
      bool isActive = false;
      if (denominator > 0)
      {
        double ratio = numerator / denominator;
        double threshold = abs(Threshold);
    
        isActive = ((ratio > threshold) || (ActiveIfEqual && (ratio == threshold)));
      }
    
      SetActive(isActive);
    }

    Анализатор указал на подозрительное выражение ratio == threshold. Эти переменные типа double и точное сравнение таких переменных простым оператором равенства вряд ли возможно. Тем более, что значение переменной ratio получено после операции деления.

    Это очень странный код для приложения «Калькулятор». Поэтому прикладываю весь список предупреждений этого типа:
    • V550 An odd precise comparison. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 752
    • V550 An odd precise comparison: stod(roundedString) != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcManager UnitConverter.cpp 778
    • V550 An odd precise comparison. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 790
    • V550 An odd precise comparison: stod(roundedString) != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcManager UnitConverter.cpp 820
    • V550 An odd precise comparison: conversionTable[m_toType].ratio == 1.0. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 980
    • V550 An odd precise comparison: conversionTable[m_toType].offset == 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcManager UnitConverter.cpp 980
    • V550 An odd precise comparison: returnValue != 0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcManager UnitConverter.cpp 1000
    • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 270
    • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 289
    • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 308
    • V550 An odd precise comparison: sizeToUse != 0.0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. CalcViewModel LocalizationService.cpp 327
    • V550 An odd precise comparison: stod(stringToLocalize) == 0. It's probably better to use a comparison with defined precision: fabs(A — B) < Epsilon. CalcViewModel UnitConverterViewModel.cpp 388

    Подозрительная последовательность функций


    V1020 The function exited without calling the 'TraceLogger::GetInstance().LogNewWindowCreationEnd' function. Check lines: 396, 375. Calculator App.xaml.cpp 396
    void App::OnAppLaunch(IActivatedEventArgs^ args, String^ argument)
    {
      ....
      if (!m_preLaunched)
      {
        auto newCoreAppView = CoreApplication::CreateNewView();
        newCoreAppView->Dispatcher->RunAsync(....([....]()
        {
          TraceLogger::GetInstance().LogNewWindowCreationBegin(....); // <= Begin
          ....
          TraceLogger::GetInstance().LogNewWindowCreationEnd(....);   // <= End
        }));
      }
      else
      {
        TraceLogger::GetInstance().LogNewWindowCreationBegin(....);   // <= Begin
    
        ActivationViewSwitcher^ activationViewSwitcher;
        auto activateEventArgs = dynamic_cast<IViewSwitcherProvider^>(args);
        if (activateEventArgs != nullptr)
        {
          activationViewSwitcher = activateEventArgs->ViewSwitcher;
        }
    
        if (activationViewSwitcher != nullptr)
        {
          activationViewSwitcher->ShowAsStandaloneAsync(....);
          TraceLogger::GetInstance().LogNewWindowCreationEnd(....);   // <= End
          TraceLogger::GetInstance().LogPrelaunchedAppActivatedByUser();
        }
        else
        {
          TraceLogger::GetInstance().LogError(L"Null_ActivationViewSwitcher");
        }
      }
      m_preLaunched = false;
      ....
    }

    Диагностика V1020 анализирует блоки кода и эвристически пытается найти ветви, в которых забыт вызов функции.

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

    Ненадёжные тесты


    V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. CalculatorUnitTests UnitConverterViewModelUnitTests.cpp 500
    public enum class NumbersAndOperatorsEnum
    {
      ....
      Add = (int) CM::Command::CommandADD,   // 93
      ....
      None = (int) CM::Command::CommandNULL, // 0
      ....
    };
    
    TEST_METHOD(TestButtonCommandFiresModelCommands)
    {
      ....
      for (NumbersAndOperatorsEnum button = NumbersAndOperatorsEnum::Add;
           button <= NumbersAndOperatorsEnum::None; button++)
      {
        if (button == NumbersAndOperatorsEnum::Decimal ||
            button == NumbersAndOperatorsEnum::Negate ||
            button == NumbersAndOperatorsEnum::Backspace)
        {
          continue;
        }
        vm.ButtonPressed->Execute(button);
        VERIFY_ARE_EQUAL(++callCount, mock->m_sendCommandCallCount);
        VERIFY_IS_TRUE(UCM::Command::None == mock->m_lastCommand);
      }
      ....
    }

    Анализатор обнаружил цикл for, в котором не выполняется ни одна итерация, а, следовательно, не выполняются и тесты. Начальное значение счётчика цикла button (93) сразу превышает конечное (0).

    V760 Two identical blocks of text were found. The second block begins from line 688. CalculatorUnitTests UnitConverterViewModelUnitTests.cpp 683
    TEST_METHOD(TestSwitchAndReselectCurrentlyActiveValueDoesNothing)
    {
      shared_ptr<UnitConverterMock> mock = make_shared<UnitConverterMock>();
      VM::UnitConverterViewModel vm(mock);
      const WCHAR * vFrom = L"1", *vTo = L"234";
      vm.UpdateDisplay(vFrom, vTo);
      vm.Value2Active = true;
      // Establish base condition
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
      vm.Value2Active = true;
      VERIFY_ARE_EQUAL((UINT)1, mock->m_switchActiveCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_sendCommandCallCount);
      VERIFY_ARE_EQUAL((UINT)1, mock->m_setCurUnitTypesCallCount);
    }

    Ещё один тест с подозрительным кодом. Анализатор обнаружил идентичные фрагменты кода, выполняющиеся друг за другом. Возможно, код писался копированием этих фрагментов, но программист забыл изменить часть кода.

    V601 The 'false' value is implicitly cast to the integer type. Inspect the second argument. CalculatorUnitTests CalcInputTest.cpp 352
    Rational CalcInput::ToRational(uint32_t radix, int32_t precision) { .... }
    
    TEST_METHOD(ToRational)
    {
      ....
      auto rat = m_calcInput.ToRational(10, false);
      ....
    }

    В функцию ToRational передают булевское значение false, хотя параметр имеет тип int32_t и называется precision.

    Я решил отследить используемое значение в коде. Далее оно передаётся в функцию StringToRat:
    PRAT StringToRat(...., int32_t precision) { .... }

    а затем в StringToNumber:
    PNUMBER StringToNumber(...., int32_t precision)
    {
      ....
      stripzeroesnum(pnumret, precision);
      ....
    }

    И вот тело нужной функции:
    bool stripzeroesnum(_Inout_ PNUMBER pnum, long starting)
    {
      MANTTYPE *pmant;
      long cdigits;
      bool fstrip = false;
    
      pmant=pnum->mant;
      cdigits=pnum->cdigit;
      
      if ( cdigits > starting ) // <=
      {
        pmant += cdigits - starting;
        cdigits = starting;
      }
      ....
    }

    Тут мы видим, что переменная precision стала называться starting и участвует в выражении cdigits > starting, что очень странно, ведь туда изначально передали значение false.

    Избыточность


    V560 A part of conditional expression is always true: NumbersAndOperatorsEnum::None != op. CalcViewModel UnitConverterViewModel.cpp 991
    void UnitConverterViewModel::OnPaste(String^ stringToPaste, ViewMode mode)
    {
      ....
      NumbersAndOperatorsEnum op = MapCharacterToButtonId(*it, canSendNegate);
    
      if (NumbersAndOperatorsEnum::None != op)      // <=
      {
        ....
        if (NumbersAndOperatorsEnum::None != op &&  // <=
            NumbersAndOperatorsEnum::Negate != op)
        {
          ....
        }
        ....
      }
      ....
    }

    Переменная op уже сравнивалась со значением NumbersAndOperatorsEnum::None и дублирующую проверку можно удалить.

    V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression. Calculator Calculator.xaml.cpp 239
    void Calculator::AnimateCalculator(bool resultAnimate)
    {
      if (App::IsAnimationEnabled())
      {
        m_doAnimate = true;
        m_resultAnimate = resultAnimate;
        if (((m_isLastAnimatedInScientific && IsScientific) ||
            (!m_isLastAnimatedInScientific && !IsScientific)) &&
            ((m_isLastAnimatedInProgrammer && IsProgrammer) ||
            (!m_isLastAnimatedInProgrammer && !IsProgrammer)))
        {
          this->OnStoryboardCompleted(nullptr, nullptr);
        }
      }
    }

    Это гигантское условное выражение изначально имело ширину 218 символов, но я разбил его на несколько строк для демонстрации предупреждения. А переписать код можно до такого короткого и, главное, читабельного варианта:
    if (   m_isLastAnimatedInScientific == IsScientific
        && m_isLastAnimatedInProgrammer == IsProgrammer)
    {
      this->OnStoryboardCompleted(nullptr, nullptr);
    }

    V524 It is odd that the body of 'ConvertBack' function is fully equivalent to the body of 'Convert' function. Calculator BooleanNegationConverter.cpp 24
    Object^ BooleanNegationConverter::Convert(....)
    {
        (void) targetType;    // Unused parameter
        (void) parameter;    // Unused parameter
        (void) language;    // Unused parameter
    
        auto boxedBool = dynamic_cast<Box<bool>^>(value);
        auto boolValue = (boxedBool != nullptr && boxedBool->Value);
        return !boolValue;
    }
    
    Object^ BooleanNegationConverter::ConvertBack(....)
    {
        (void) targetType;    // Unused parameter
        (void) parameter;    // Unused parameter
        (void) language;    // Unused parameter
    
        auto boxedBool = dynamic_cast<Box<bool>^>(value);
        auto boolValue = (boxedBool != nullptr && boxedBool->Value);
        return !boolValue;
    }

    Анализатор обнаружил две функции, которые реализованы одинаково. По названиям функций Convert и ConvertBack можно предположить, что они должны выполнять разные действия, но разработчикам виднее.

    Заключение


    Наверное, каждый открытый проект от Microsoft давал нам возможность показать важность применения методологии статического анализа. Даже на таких маленьких проектах, как калькулятор. В таких крупных компаниях, как Microsoft, Google, Amazon и других, работает много талантливых программистов, но они такие же люди, которые делают ошибки в коде. Применение инструментов статического анализа кода — один из хороших способов повысить качество программ в любых командах разработчиков.

    Проверь свой «Калькулятор», скачав PVS-Studio и попробовав на своём проекте. :-)



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Counting Bugs in Windows Calculator
    PVS-Studio
    589,85
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

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

      +24
      Сравнение строк указателей, не освобождённый буфер — такие ошибки выглядят так, как буд-то над кодом местами поработал студент-практикант.
        +16
        а так вполне возможно и было — в MS много интернов, которым отдают в разработку некритичные части софта.
          +3
          Юниоры писали.
            +4

            Очень может быть, что писали шарписты, которых заставили реюзать плюсовый код и потому писать на C++/CX. Отсюда, в частности, и ошибки с приватным наследованием, и итерирование энума.

              0
              Скорее всего, бОльшая часть кода написана ещё до появления C#, всё же
                0

                Код ядра калькулятора тянется ещё с 1989 года, если верить исходникам.

                  –1
                  Хочу видеть историю их гита с 1989 года. Что пушилось, что мерджилось, где в форки ушло, и в какие форки. Когда происходил ремастер, т.к. в 10 калькулятор довольно сильно отличается от 95.
                    +7

                    Гит появился только в 2005 :) В любом случае вряд ли когда-либо MS выложит полную историю правок исходников, даже если она у них и осталась.

                      +1
                      К сожалению — да, но даже с 2005 года будет интересно.
                      Что не понятно, почему минусов за желание увидеть, пусть и синтезированную гит историю насыпалось.
                        +1

                        "Хочу видеть" это просто неправильный посыл с т.з. Хабра. Все мы чего-то хотим.

                          0
                          пусть и синтезированную гит историю

                          Синтезировать можно хоть с 1970-го.
              +1
              Ну глядя на калк в последней винде, в этом можно быть уверенным на 100%. По крайней мере в части, отвечающей за интерфейс…
                +1

                Ну а что. Ядро написано. Отлажено за 30 лет. В компанию приходит джун, ему менеджер говорит: «Так, нам надо сделать для калькулятора новый UI. Писать с нуля не нужно — есть исхожники старого. Просто сделай на C++/CX интерфейс к нему». Вот он и написал, проецируя на C++ свой опыт в C#.

              +11
              «В таких крупных компаниях, как Microsoft, Google, Amazon и других, работает много талантливых программистов, но...» не только они :)
              Спасибо, Святослав, за оперативность — спасибо Микрософту за рабочую субботу? :)
                +8
                Я ещё в пятницу начал)
                  +3
                  Я ещё в пятницу начал)
                  Спасибо Майкрософту за рабочий праздник? :)
                +9
                очень маленькая программа на языке C++

                тут потерялась табличка сарказм, или 35 KSLOC сегодня считается маленькой программой (в частности, калькулятором)?
                  +17
                  Возможно, этот калькулятор крупнее обычного, согласен. Но с C++ сталкиваюсь постоянно, 35 — небольшой проект.
                  +6
                  На мой взгляд не так уж и плохо: я опасался, что сравнение указателей это лишь вершина айсберга. Как в старом анекдоте: «Ну да, ужас. Но совсем не УЖАС-УЖАС-УЖАС!»
                    –34
                    Данная статья наглядно демонстрирует чрезмерный уровень проверки качества кода инструментом. Очевидно, что калькулятор за 25 лет существования был избавлен от пользовательски-значимых ошибок. При этом тестируемый анализатор кода показывает наличие множества десятистепенных ошибок, которые не нужно исправлять — их исправление не принесёт пользу пользователям, а значит лишь увеличит затраты на поддержку калькулятора.
                      +19

                      Насколько я понял, это калькулятор из Windows 10, не думаю, что ему 25 лет

                      +4
                      Очевидно, что калькулятор за 25 лет существования был избавлен от пользовательски-значимых ошибок.

                      откуда уверенность, что в выложенном куркуляторе много кода из предыдущих версий?
                        +3
                        Я посмотрел и уверен, что их там нет вообще.
                        +24
                        При этом тестируемый анализатор кода показывает наличие множества десятистепенных ошибок, которые не нужно исправлять — их исправление не принесёт пользу пользователям, а значит лишь увеличит затраты на поддержку калькулятора.

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

                        Неэффективность правки старых ошибок, не означает неэффективность методологии статического анализа в целом.

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

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

                        P.S. В PVS-Studio даже есть режим, когда он показывает ошибки только в новом и отредактированном коде. А всё старое, считается техническим долгом, с которым можно неспешно работать (или вообще не работать :).
                          +1
                          P.S. В PVS-Studio даже есть режим, когда он показывает ошибки только в новом и отредактированном коде. А всё старое, считается техническим долгом, с которым можно неспешно работать (или вообще не работать :).

                          В PVS под Linux тоже есть такая возможность?
                        0
                        [del]
                          +26
                          Такие ошибки могут спокойно жить в ядре системы и в критических сервисах, точно также по 10-15 лет а потом бац и очередной WannaCry стучится в порт.
                            0
                            А зачем вы написали кусок кода, в котором наличие ошибки ни на что не влияет?
                            Тут явно статическим анализом не отделаешься…
                              –11
                              «был избавлен от пользовательски-значимых ошибок»
                              Тавышо! А ничего что калькулятор из метро приложений не умеет правильно считать? Я не прикалываюсь. Задайте ему непосильную задачу (2х2)*2 увидите много интересного.
                                +1

                                Это есть и в старом калькуляторе, и было "объяснение, почему он считает по другому(типа не инженерный калькулятор), на инженерном виде все правильно считает.
                                Вроде задачка выглядела по другому (2+2)*2

                                  +1
                                  2 + 2 * 2, без скобок. Должно получиться 6, если калькулятор знает о порядке действий, может получиться 8, если не знает
                                    +9
                                    Это, конечно, флейм-баян, но калькулятору в винде не положено знать о порядке действий.
                                    Он имитирует древние «бухгалтерские» кнопочные калькуляторы, которые не имели никакого буфера и при нажатии следующей операции выполняли предыдущую и использовали ее результат как первый операнд.
                                      +2

                                      В инженерном режиме с порядком действий всё нормально (ответ 6), в обычном режиме — работает по бухгалтерскому варианту (ответ 8)

                                        +3
                                        Если переключить калькулятор в режим «Инженерный» или «Программист», то всё считается правильно. Это не баг, это фича.
                                  +5

                                  Вот, к примеру, мы пишем Java IDE. Вчера благодаря статическому анализу я обнаружил ошибку в процедуре рефакторинга Inline method. Код, на котором она проявляется, следующий (пытаемся инлайнить метод getTwice):


                                  class Test {
                                      enum Foo {
                                          BAR(getTwice(() -> {
                                              return getNum();
                                          }));
                                  
                                          Foo(int val) {}
                                      }
                                  
                                      static int getNum() {
                                          return 4;
                                      }
                                  
                                      static int getTwice(IntSupplier fn) {
                                          int x = fn.getAsInt();
                                          int y = fn.getAsInt();
                                          return x + y;
                                      }
                                  }

                                  Чтобы баг проявился, необходимо выполнение следующих условий:


                                  • Метод, который мы инлайним, должен возвращать значение
                                  • Метод, который мы инлайним, должен быть сложнее, чем просто один return, должно быть минимум два оператора в его теле
                                  • Метод, который мы инлайним, должен вызываться в конструкторе enum-константы
                                  • Среди аргументов этого метода должно быть лямбда-выражение
                                  • В лямбда-выражении должен быть полноценный return (простая expression-lambda не подойдёт)
                                  • return в этой лямбде должен возвращать результат другого метода (не константу, не арифметическую операцию, не переменную, а именно результат метода)

                                  Только при выполнении всех условий рефакторинг не срабатывает, а IDE выдаёт исключение. Рефакторингу больше 15 лет, им пользовались миллионы раз в самых разнообразных проектах, никто никогда не сообщил об этой проблеме. Возможно, никто просто ещё не написал настолько извратный Java-код. А статический анализатор играючи указал на ошибку в нашем коде. Исправление тривиальное, а вот чтобы сконструировать тест-кейс, где ошибка проявляется, я потратил около получаса. Стоило ли исправлять или дожидаться недовольного пользователя? Как считаете?

                                  +1
                                  Это сколько же у них тогда ошибок в Офисе?
                                    +14
                                    Столько же, сколько и в другом подобном крупном проекте.
                                    +12

                                    V547 Expression 'm_resolvedName == L«en-US»' is always false.
                                    Шах и мат:



                                    (ничего не менял, склонировал, запустил, поставил бряку)
                                    is always false говорите?

                                      +1
                                      Вчера также отлаживал, не удалось войти в эту ветку.
                                        +1

                                        Ох какая непостоянная ветка, представьте себе, в C# тоже строки по ссылкам в первую очередь сравнивают :-))

                                          +1
                                          В C# представьте себе все ref объекты сравниваются в первую очередь по ссылкам, а ещё есть такое понятие как интернирование строк.
                                            +2
                                            В C# для строк перегружен оператор `==`.
                                            Скорее всего, тут приколы со string interning.
                                          +42
                                          Коллеги, исходный код уже изменён в репозитории! Будьте внимательны при игре в шахматы)
                                          • НЛО прилетело и опубликовало эту надпись здесь
                                              +1
                                              Ммм… Возможно, но прошу дать ссылку на стандарт или другой документ где это уточняется. Сравниваются на равенство два указателя на wchat_t.
                                                +7
                                                С точки зрения C/C++ это «always false».

                                                Однако, в «военное время» компилятор (или даже линкер) может объединять константы (в том числе константные строки), и вот тогда сравнение будет работать правильно. Причем подобное не редкость в embedded-проектах.
                                                  0
                                                  И такая же фигня и в Java'у перекочевала:
                                                      public static void main(String[] args) {
                                                          String s1 = "test";
                                                          String s2 = "test";
                                                          if (s1 == s2) {
                                                              System.out.println("OMG they equals");
                                                          }
                                                          else {
                                                              System.out.println("You should always use String.equals when you want to compare strings for equality");
                                                          }
                                                      }
                                                  


                                                  Что вот выведет?
                                                    +3
                                                    Формально это неопределенное поведение. Т.е. результат будет сильно варьироваться от JVM, т.к. вопрос в том, есть ли и будет ли работать string pool.

                                                    HotSpot скажет True по дефолту, если просто запустить этот код, но нужно понимать, что string pool это такой себе tunable cache.

                                                    Поэтому его работа описывается «may not be cached».
                                                      +9

                                                      Серьезно? Формально, одинаковые строковые литералы в Java всегда равны по ссылке (пункт 3.10.5 language spec).


                                                      Можете привести пример JVM или настроек, при которых код выше выведет «You should always use String.equals when you want to compare strings for equality»?

                                                        0
                                                        Я когда писал приложение на Андроид случайно сравнил одинаковые строки через ==
                                                        Они оказались неравны.
                                                        Хотя это были переменные разных объектов.
                                                        Так что дело либо в этом, либо в Google JVM
                                                          +2

                                                          Именно, что переменные, а не литералы.

                                                        +5
                                                        Цитата из JLS 3.10.5

                                                        A string literal is a reference to an instance of class String (§4.3.1, §4.3.3).

                                                        Moreover, a string literal always refers to the same instance of class String. This is because string literals — or, more generally, strings that are the values of constant expressions (§15.28) — are «interned» so as to share unique instances, using the method String.intern (§12.5).

                                                        Там же примеры, демонстрирующие это поведение.

                                                        Я считаю, что в Java конкретно это поведение вполне определено. Более того, я уверен, что в дикой природе множество кода завязано именно на это поведение (используют строковые литералы вместо Enum-ов и полагаются на быстрое сравнение указателей для производительности) и это поведение никогда не будет изменено.
                                                          0
                                                          String::equals уже по умолчанию использует сравнение по указателю для производительности, как в Oracle JDK, так и в Open JDK и «даже» в Android JDK. Если сорить такими сравнениями ещё и за пределами equals, в собственном коде, то статистически максимальный выигрыш будет стремиться к стоимости вызова единственного метода. Стоит ли овчинка выделки с учётом всей машинерии внутри — вопрос более чем открытый.
                                                    • НЛО прилетело и опубликовало эту надпись здесь
                                                        0

                                                        На всякий случай, всё-таки поясню почему вы не правы.


                                                        Если компилятору разрешить объединять константы (см ниже), то он вправе совместить все байты всех констант, с учетом требований к выравниванию их типов. Более того, аналогичный функционал может быть параллельно или независимо реализован в линкере.


                                                        В рассматриваемом случае такое объединение точно произойдет, если где-то в пределах видимости компилятора (или "умного" линкера) есть константа с совпадающим названием локали.
                                                        В свою очередь, если m_resolvedName имеет тип const wchat_t* и соответствующее значение указателя было сохранено (не было создано исходной константы), то и результат сравнения будет true.


                                                        Всё вышеописанное повсеместно в коде проектов собираемых для "мелких" процессоров (PIC, ATmega и т.п.).


                                                        -fmerge-constants
                                                        Attempt to merge identical constants (string constants and floating-point constants) across compilation units.

                                                        This option is the default for optimized compilation if the assembler and linker support it. Use -fno-merge-constants to inhibit this behavior.

                                                        Enabled at levels -O, -O2, -O3, -Os.

                                                        -fmerge-all-constants
                                                        Attempt to merge identical constants and identical variables.

                                                        This option implies -fmerge-constants. In addition to -fmerge-constants this considers e.g. even constant initialized arrays or initialized constant variables with integral or floating-point types. Languages like C or C++ require each variable, including multiple instances of the same variable in recursive calls, to have distinct locations, so using this option results in non-conforming behavior.
                                                        • НЛО прилетело и опубликовало эту надпись здесь
                                                      –1
                                                      Я в статье-обзоре дал линк, так минусовать начали. За что — не понятно.
                                                      habr.com/ru/post/443018/#comment_19854498
                                                      Там написано же у M$ почему так работает
                                                      0

                                                      Я как то читал давно. Что из за того что многие в функцию принт передавали std string и их вариант из mfc то ms просто поддержали и эти случаи, хотя это и ub. Возможно тут тоже компилятор заменяет ub на то что ожидает пользователь. Но это не точно.

                                                        +1
                                                        Это навреное была моя статья "Большой брат помогает тебе".
                                                        В данном случае никакого UB нет. Один указатель на wchar_t сравнивается с другим указателем. Вот и всё. А вся дискуссия пошла из-за того, что в заголовочном файле массив символов m_resolvedName уже превратился в полноценную строку типа std::wstring. И теперь сравнение работает правильно.
                                                        +2
                                                        Потому что эта ситуация не «always false», а «undefined behaviour».
                                                        Никак нет, потому что в коде
                                                        wchar_t buffer[100] = L"test";
                                                        if (buffer == L"test")

                                                        компилятор не имеет права этим двум строкам дать один адрес, т.к. у них разная const-ность.
                                                          +1

                                                          Почему это не имеет права? В современных компиляторах спецификатор const используется только для контроля доступа и выбора нужных перегрузок функций, после чего он игнорируется. Компилятор дальше сам определяет, возможна ли модификация области памяти или нет. И если модификация невозможна, то почему бы не сэкономить на памяти?

                                                          • НЛО прилетело и опубликовало эту надпись здесь
                                                              +1
                                                              Кто ж вам виноват, что здесь вы сами посадили UB.
                                                              Так можно: wchar_t buffer[100] = L"test";
                                                              А так нельзя: wchar_t* buffer = L"test";

                                                              ISO C++ 11 does not allow conversion from string literal to 'wchar_t*' [clang-diagnostic-writeable-strings]

                                                              А вот так можно: const wchar_t* buffer = L"test";
                                                                +2
                                                                Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу. Раз в сообщении выше показано, что точка останова сработала, значит как раз ссылка на константу и была передана в функцию
                                                                А посмотреть исходник? Благо даже скачивать не надо, github позволяет браузить репозиторий online. В оригинале был массив
                                                                wchar_t m_resolvedName[100];
                                                                  +8
                                                                  Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу.

                                                                  Ох… Ну сколько можно… В статье чётко написано:
                                                                  wchar_t m_resolvedName[LOCALE_NAME_MAX_LENGTH];
                                                                  ...
                                                                  if (m_resolvedName == L"en-US")
                                                                  

                                                                  Такое условие всегда false.

                                                                  Для кода:
                                                                  wchar_t * buffer = L"test";
                                                                  if (buffer == L"test")

                                                                  Условие может сработать. Но это совсем другой случай. Сравнение может дать true, но это зависит от везения и фазы луны.

                                                                  P.S. Условие if (m_resolvedName == L«en-US») сработало из-за того, что код уже успели поправить, заменив простой массив символов на std::wstring. Уже несколько раз это обсудили в комментариях.

                                                                  • НЛО прилетело и опубликовало эту надпись здесь
                                                                      +2
                                                                      Вы осознаёте, что
                                                                      const wchar_t *A = L"test";
                                                                      wchar_t B[] = L"test";
                                                                      
                                                                      принципиально разные сущности в C++?
                                                                      • НЛО прилетело и опубликовало эту надпись здесь
                                                                          +1
                                                                          В статьях мы тщательно подходим к выписыванию фрагментов кода, делая их по возможности самодостаточными для демонстрации ошибки. С самого начала в статье приводился следующий фрагмент кода:
                                                                          wchar_t m_resolvedName[LOCALE_NAME_MAX_LENGTH];
                                                                          
                                                                          Platform::String^ GetEnglishValueFromLocalizedDigits(....) const
                                                                          {
                                                                            if (m_resolvedName == L"en-US")
                                                                            {
                                                                              return ref new Platform::String(localizedString.c_str());
                                                                            }
                                                                            ....
                                                                          }

                                                                          Из этого пример можно видеть, что такое m_resolvedName. Мы поспешили и не проговорили как следует это текстом и скомкано описали ошибку. Это признаём. Но не надо свою собственную невнимательность превращать в:
                                                                          В первоначальной редакции статьи никакого указания, что передаётся массив, не было. Поменяли, теперь стало иначе.


                                                                          P.S. Эффект, возникающий из-за пула строк, кратко затронут в документации (семилетней давности) на диагностику V547. Только не говорите, что и ссылки на документацию тоже не было :).
                                                                          • НЛО прилетело и опубликовало эту надпись здесь
                                                                              +3
                                                                              2. С массивом тоже будет «undefined behaviour», а не «always false».
                                                                              Объясните, почему?
                                                                              Массив — член класса его адрес считается как смещение поля от адреса объекта. Строковый литерал — статическое значение. Не важно, размещено в пуле или ещё где-то в секции read-only data. Почему адреса могут совпасть? Разве в стандарте запрещено сравнение указателя с литералом? Вероятно, тут CLion не прав.
                                                                              • НЛО прилетело и опубликовало эту надпись здесь
                                                                                  +2
                                                                                  Ленивые присваивания строк.
                                                                                  Это касается только классов-обёрток (типа std::string), но никак не Си-шных «строк» (char*)
                                                                                  • НЛО прилетело и опубликовало эту надпись здесь
                                                                      –1
                                                                      Скорее всего не понадобится ни везение, ни фазы луны. VS использует string pooling при настройках по-умолчанию что в debug, что в release сборке.
                                                                        –1
                                                                        Тут речь о том, что сообщение PVS-Studio некорректное или неполное, что привело к неправильной интерпретации этого сообщения даже у вас в статье. Сравнение не будет всегда false для сравнения указателей(!), это показывает практика string pooling в VS. В данном конкретном случае проблема не в сравнении указателей, а в том, что m_resolvedName это массив и в С++ нужно очень сильно извратиться, чтобы условие сработало, если это вообще возможно. В данном случае PVS-Studio должна была написать именно это — переменная массив и условие потенциально никогда не выполнится.

                                                                        А вот если бы m_resolvedName было простым wchar_t*, то условие очень даже сработает, если есть string pooling или аналог в данном конкретном компиляторе. В этом случае PVS-Studio стоит предупредить, что это потенциально непереносимый код, который зависит от конфигурации и типа компилятора. Работать будет, но лучше исправить.
                                                                          +1
                                                                          В сообщении сложно уместить описание проблемы. Однако, этот вопрос рассматривается в документации к диагностике V547. А в статье просто не аккуратно написано, потому что очень торопились. Перед переводом подретушируем. Не хватает вокруг этого случая ещё и дискуссии на англоязычных форумах устраивать… :)
                                                                            0
                                                                            «m_resolvedName is an array» очень легко уместить и это, я думаю, сняло бы вопросы и утверждение «always false» было бы действительно верно. Опять же, будь переменная простым указателем, диагностика была бы вообще некорректная от слова совсем. В документации упоминается пулинг строк, но не упоминается ситуация с массивами, которые в С++ ведут себя совсем иначе, нежели в С.

                                                                            Как я понял, товарищ Delics именно это и хотел донести, но его почему-то минусуют.
                                                                              +1
                                                                              В данном случае PVS-Studio должна была написать именно это — переменная массив и условие потенциально никогда не выполнится.
                                                                              Может, для кого-то и надо так разжёвывать, но как по мне «expression is always false» тут самое лучшее. Про массивы сложно было бы понять, что мне хочет сказать программа.

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

                                                                              А я вот проверил. И действительно, PVS-Studio пишет то же самое сообщение «V547 Expression '...' is always false. To compare strings you should use wcscmp() function». Так что Andrey2008, справедливо отметивший, что wchar_t* и wchar_t[] в C++ разные вещи, теперь должен объяснить это своей программе )))

                                                                              Кстати, конкурент в этом кейсе более корректен — пишет UB:
                                                                              Result of comparison against a string literal is unspecified (use strncmp instead) [clang-diagnostic-string-compare]
                                                                              Единственное замечание, что он предлагает сравнивать wide-строки через strncmp, а не wcscmp (и почему strncmp с длиной, это не замена оператору == ).
                                                                                0
                                                                                Может, для кого-то и надо так разжёвывать, но как по мне «expression is always false» тут самое лучшее

                                                                                Тут оно действительно лучшее, если бы второй кейс имел другое описание. С моей стороны была догадка, но, получается, она сбылась и какое-то из диагностических сообщений надо исправлять. Проще всего конечно «always false» заменить на UB и ничего не надо объяснять никому.
                                                                                  +2
                                                                                  Писать UB в случае, когда оно не UB — проще, но не правильно.
                                                                              0
                                                                              Не комплексуйте. Устраивайте дискуссии.
                                                                              Их на самом деле не… бурили достаточно. Очень похожую ошибку как — то нашел у них в документации. Они её поправили, а мне в ответ пишут «Ваше замечание бессмысленно». Моя попытка напомнить публике, что надо сказать «спасибо» натолкнулась на корпоративное «фи», мою учетную запись забанили.
                                                                              Так что понаглее, локти по-шире и коленкой вперед! :)))))))))))
                                                                              Удачи.
                                                                    –2
                                                                    Так undefined behavior же
                                                                      +10
                                                                      Обратите внимание на сигнатуры в вашем коде — wstring против wchar_t из статьи. wstring поддерживает структурное равенство через ==, массив wchar_t — нет.
                                                                      Именно такие малозаметные изменения и опечатки говорят в пользу статического анализа кода.
                                                                        0

                                                                        Ставлю сотку приложение спрототипировали на C# и потом переписали на С++ CLI/CX, отсюда таки перлы, благо незаход в ветку никак не ломал приложение — это была лишь оптимизация насколько я понял.

                                                                          0
                                                                          Ставлю сотку приложение спрототипировали на C# и потом переписали на С++ CLI/CX
                                                                          Но зачем?
                                                                        +2
                                                                        А вот коммит, который это поменял — github.com/Microsoft/calculator/commit/c85d7ec4549ff06a7b7dc08201c671336725fba8
                                                                        И сделано это буквально вчера.
                                                                          +7
                                                                          Хороший ответ на «эти ошибки никому не мешают». Человек не поленился в выходной поправить ошибку в рабочем проекте.
                                                                            +2
                                                                            Ну стыдно же:) Если в вашем проекте такое найдут и в новостях напишут, я думаю вы тоже подорветесь править.
                                                                              +4
                                                                              Мне только интересно, эти исправления сделаны на публику в гитхабе, или они попали во внутренние репозитории MS, из которых собирается продукт. Узнаем со следующим апдейтом, будет ли туда включен calc.exe.
                                                                                +3
                                                                                в выходной

                                                                                В США 8 марта выходным не является. Поэтому все же в обычный рабочий день

                                                                                  0
                                                                                  Deleted
                                                                            0

                                                                            А перевод этой статьи на английский есть? Порылся в вашем блоге, но не нашел.

                                                                              +2
                                                                              Ещё нет, но будет. Решили опубликовать пока идёт бурное обсуждение новости, не дожидаясь перевода.
                                                                                +3
                                                                                Я опубликовал свеженаписаннаю статью. Перевод подъедет чуть позже.
                                                                                  0
                                                                                  Планируете еще и на хабр перевод выложить? С открытием англоязычного хабра это имеет смысл.
                                                                                +1
                                                                                Подготовили перевод:


                                                                                Следите за новостями. Скоро подъедут сравнения с другими популярными калькуляторами)
                                                                                +3

                                                                                Интересно было бы теперь для сравнения проверить код юниксового калькулятора bc.

                                                                                  +8
                                                                                  Вы можете это сделать абсолютно самостоятельно, чтобы была статья от независимого автора. Так будет даже интереснее! И PVS-Studio доступна, и исходники калькулятора есть. Поможете нам сделать серию статей о проверке калькуляторов? ;-)
                                                                                    +3

                                                                                    Спасибо за идею, обязательно попробую. Заодно освою новый для себя инструмент. Честно говоря, не знал о существовании триальной версии PVS :)

                                                                                      0
                                                                                      Она великолепна. Там и статейки сразу же есть с новостями внутри.
                                                                                • НЛО прилетело и опубликовало эту надпись здесь
                                                                                    –7
                                                                                    Прикольно. В MS это писали «студенты». Как повысить качество кода? Выложить в открытый доступ. Тут же набросились профессионалы, и вуаля — код улучшился. Красиво и дёшево.
                                                                                      0
                                                                                      Ну нашли 100500 банальных опечаток. Бизнес-пользы в этом никакой, разве что платить дополнительно сотрудникам, которые промониторят найденные ошибки и внесут исправления в репозиторий.

                                                                                      Другое дело, если бы кто-то нашёл архитектурные ошибки и глобально отрефакторил — тогда бы код улучшился и его дешевле было бы развивать в будущем. Но что-то таких героев не видно.
                                                                                      +5
                                                                                      Главная мысль это то что для коммерческого успеха идеальный код не нужен. Это на заметку собеседющим, главное это человеческие качества, имхо основное это умение довести до конца.
                                                                                        +2
                                                                                        Ну и приложение из стандартной поставки Windows это тоже не «коммерческий успех», прошу заметить — кто его самостоятельно ставить-то или покупать будет? Так что мысль хорошая, только далековата от предмета статьи =)
                                                                                          0
                                                                                          Стандартная поставка влияет на коммерческий успех Windows. Другое дело что калькулятор вряд ли вносит хоть сколько-то заметный вклад в этот успех, поэтому практически вы правы.
                                                                                        +1
                                                                                        А что это за интересный синтаксис:
                                                                                        String^
                                                                                          +3
                                                                                          Это C++/CLI.
                                                                                            +2
                                                                                            Спасибо. Оказывается добавляет ref-поведение, необходимое для работы в .NET:
                                                                                            This feature is necessary to give a ref class the semantics for operator overloading expected from .NET ref classes. (In reverse, this also means that for .NET framework ref classes, reference operator overloading often is implicitly implemented in C++/CLI.)
                                                                                          –17
                                                                                          Докатились. Калькулятор не можем написать без кучи багов и неоднозначного кода.
                                                                                          А сразу правильно что не пишется? Благо логики в калькулаторе минимум.
                                                                                            0
                                                                                            Похоже, калькулятор это первые шаги перед переводом чего-то большого в опенсорс. Так же было вначале с .net, сначала библиотеки, unit тесты и тд.
                                                                                              +2

                                                                                              Выложат ядро винды в опенсорс. На kernel.com

                                                                                                +4
                                                                                                … и к нему тоже заведут issue #1 «Failed to compile on Linux»?
                                                                                                  0
                                                                                                  К тому времени они MSVC отопенсорсят и под линукс портируют. С# компилятор уже там.
                                                                                              +1

                                                                                              у меня комп флешку не отдаёт пока калькулятор не закроешь в некоторых случаях (похоже когда в тотал коммандере открыта флешка и я запускаю с панели тотала иконку калькулятора)

                                                                                                +11
                                                                                                Видимо, тотал ставит калькулятору флешку в качестве текущей директории, отчего она становится занятой.
                                                                                                +6
                                                                                                Поэтому я когда-то забросил С++ и перешел на Java. Ибо слишком глуп и невнимателен для плюсов. Легкость с которой можно сделать ошибку сравнима с игрой в super meat boy на хардкорных настройках. Особенно когда ты пытаешься вклинить свой код в огромный проект, написанный такими же раздолбаями как и ты сам.

                                                                                                Часто возникали ситуации «Мой код не работает — ПОЧЕМУ?!» и «Мой код работает — ПОЧЕМУ?!». И чем больше я изучал плюсы тем активнее искал из них выход.

                                                                                                Все-таки хороший плюсовик очень редкая птица. Их нужно ценить, беречь и давать молоко за вредность.
                                                                                                  0
                                                                                                  Все-таки хороший плюсовик очень редкая птица. Их нужно ценить, беречь и давать молоко за вредность.

                                                                                                  Интересно, что некоторые товарищи с Хабра, ищущие плюсовиков, считают как раз наоборот :)

                                                                                                    0
                                                                                                    Это как в классическом анекдоте: «А вы продаете или покупаете?» (:
                                                                                                  +5
                                                                                                  Это не тот калькулятор который бы мне хотелось увидеть. Последний раз нормальный калькулятор был в XP кажется. А в этом так удобно — надо тебе взять синус из числа 5Eh, ты сначала переключить в режим для программистов, конвертируй число, ЗАПОМНИ ЕГО, перейди в режим научного калькулятора (при этом число в калькуляторе очистится), введи число заново, и посчитай синус. То есть вместо двух кликов — куча ненужных действий, включая ползание по менюшкам. Какая разница, сколько в нём ошибок, если им пользоваться неудобно? И самое обидное — раньше хоть на сайте микрософта можно было скачать так называемый Calculator Plus, который точная копия того самого старого калькулятора, только зачем-то с дополнительным скином (чудовищно выглядящим и установленным по умолчанию). А теперь и его удалили. Благо хоть инсталляха сохранилась. И пусть молодёжь смеётся что хранить хлам на винте прошлый век, всё же можно скачать.
                                                                                                    +1

                                                                                                    Вот вам сорцы того самого калькулятора!

                                                                                                      0
                                                                                                      Спасибо. Было бы интересно проверить и его (есть подозрение что с ним ситуация будет ещё хуже), но для этого хорошо бы знать как его правильно компилить. Я конечно засунул его в визуал студию, и даже смог собрать что-то относительно работоспособное, но не до конца понятно где косяки исходного кода а где — мои кривые руки.
                                                                                                        +1
                                                                                                        Ну это была утечка какого-то куска Windows 2000. Никто не гарантировал, что она полная и работоспособная.
                                                                                                    +2
                                                                                                    Анализатор обнаружил две функции, которые реализованы одинаково. По названиям функций Convert() и ConvertBack() можно предположить, что они должны выполнять разные действия, но разработчикам виднее.

                                                                                                    Здесь как раз все верно. Если у нас конвертер инвертирует булево значение, то очевидно, что операция обратная инвертированию является инвертированием :) Т.е. x == !!x.

                                                                                                      +1
                                                                                                      Эта диагностика выяляет 2 проблемы:
                                                                                                      1. Функции имеют одинаковую реализацию (не всегда ошибка);
                                                                                                      2. Дублирование кода само по себе плохо. Проблемы начинаются, когда необходимо изменять код.
                                                                                                        +2
                                                                                                        2. Плохо, но в этом конкретном случае я бы выносить это в третью функцию не стал. Впрочем, анализатор, конечно, все кейсы различать не научишь.
                                                                                                          0

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

                                                                                                            0
                                                                                                            Как писавший немало конвертеров для XAML, скажу что тут нет никакого задела. BooleanNegationConverter — полностью законченная и самостоятельная вешь, нужная, если хочешь забиндить состояние чекбокса на булевскую переменную, но не на её значение, а на инверсию. Просто биндишь через этот конвертор, и всё. А что функция конверсии получается такая длинная (вместо одной строки bool result = !input), это издержки технологии, открывающие многие дополнительные возможности, когда они будут нужны.
                                                                                                        0
                                                                                                        Спасибо за статью, всегда читаю с интересом. Ребята, вы понимаете что Роскосмос, NASA и прочие должны уже в очереди стоять за вашими разработками? Кто знает сколько спутников потерялось из-за таких мелких ошибок?
                                                                                                          +2
                                                                                                          Ваши слова да Роскосмосу в уши.
                                                                                                          0
                                                                                                          Немного не по теме, но планируется ли в PVS-Studio поддержка PHP?
                                                                                                            0
                                                                                                            Пока нет. Если решим поддержать новый язык, то проведём опросник. Следите за нашим блогом)
                                                                                                              0
                                                                                                              А вообще планируете какие-нибудь языки добавлять, если не секрет конечно? И какие, если да?
                                                                                                                0
                                                                                                                3 месяца назад добавили анализатор Java. Некоторое время мы будем заниматься развитием и поддержкой существующих языков, прежде чем выберем новый.
                                                                                                                0
                                                                                                                Жаль.
                                                                                                                +1
                                                                                                                И первая же проверка на коде битрикса? ))
                                                                                                                0

                                                                                                                У меня есть предположение по эксепшинам.
                                                                                                                Возможно std:: exception был специально запривачен, что бы исключения нельзя было перехватит через catch( std::exception&).
                                                                                                                М.б. они они хотели калькулятор в виде либы сделать.
                                                                                                                А их реализация запривачена, без dll import.


                                                                                                                Ошибки с условиями очень странные.
                                                                                                                Где (а && б ||! а &&! б)…
                                                                                                                Возможно ранее, эти переменные были числами. И им нужно было проверить, что они равны или не равны нулям. А между собой, равенства не имеют. От чего, сравнение a == b не возможно.


                                                                                                                По поводу указателей.
                                                                                                                Тут наверное надо документацию по clr прочесть надо.
                                                                                                                М.б. для wchar_t и литерала L там прописаны свои правила компиляции операторов сравнения

                                                                                                                  +3
                                                                                                                  Да не было тут никакой предыстории. Просто люди ошибаются. Как выявили эти проблемы — сразу исправили. Применение статического анализа должно улучшить ситуацию.
                                                                                                                  • НЛО прилетело и опубликовало эту надпись здесь
                                                                                                                    +1
                                                                                                                    По количеству просмотров и комментариев чувствуется, что калькуляторы волнуют людей :). И автор этой статьи уже пишет заметку про калькулятор Qalculate! и делает facepalm-ы. Причина скоро станет понятна. Если совсем кратко: там всё хуже, чем в калькуляторе от Microsoft.

                                                                                                                    Но я вот к чему. Не обязательно ждать, когда мы проверим тот или иной проект. Есть различные варианты бесплатного использования PVS-Studio. Плюс можно триальную версию использовать. Можно и что-то проверить и статью написать. Или ошибки в любом проекте поправить. Подробнее: Бесплатные варианты лицензирования PVS-Studio.
                                                                                                                      0
                                                                                                                      А насколько скоро статья будет?

                                                                                                                      Кстати, по теме бесплатной версии: раньше были ограниченные переходы в бесплатной версии. Потом, в какой-то момент после переустановки системы, я поставил PVS и ограничение переходов вообще отсутствовало. Это был баг или вы ограничение убрали?
                                                                                                                        0
                                                                                                                        Ограничение переходов (кликов) относилось к демо-версии. Видимо она и была у Вас установлена. А в бесплатных версиях никакого ограничения на клики не было и нет.
                                                                                                                          0
                                                                                                                          Так я не выполнял никакие действия из бесплатной лицензии, так что обычная демо и была.
                                                                                                                          +1
                                                                                                                          Уже 2 готовы, скоро начнём постить.
                                                                                                                        0

                                                                                                                        Да, калькулятор сильно людям понравился. Вроде обычная статья от PVS-Studio, каких много, а столько плюсиков и комментариев!


                                                                                                                        An odd precise comparison: ratio == threshold

                                                                                                                        Мой опыт показывает, что эта инспекция слишком шумная и сделать её адекватной, то есть чтобы она указывала на реальные баги и не указывала на всякий мусор, довольно сложно. Но как минимум сравнения с нулём крайне редко бывают полезными предупреждениями. Я не помню ни единого случая из своей программистской практики, когда написанное программистом doubleValue == 0.0 приводило к багам, а abs(doubleValue) < someEpsilon исправляло эти баги. Рекомендую при нуле не предупреждать. Аналогично при единице. Среди тысяч найденных вами ошибок за годы работы есть хоть одна, где явное сравнение с единицей было проблемой?


                                                                                                                        Теперь вернёмся к threshold. Эта переменная имеет значение 0 во всех формах и только в форме UnitConverter имеет значение 1. Соответственно данное сравнение тоже скорее всего безопасно.


                                                                                                                        Даже не так. В случае если ActiveIfEqual == false, isActive = ratio > threshold. В случае если ActiveIfEqual == true, isActive = ratio >= threshold. Скажите, вы ругаетесь на операцию >=? Ведь здесь фактически она и есть.

                                                                                                                          0
                                                                                                                          Да, V550 (An odd precise comparison) весьма шумная диагностика. Поэтому она относится к уровню Low, который отключен по умолчанию. Однако, это одним она не интересна, а другим бывает ой как даже нужна и полезна. Цитата из статьи "О том, как мы опробовали статический анализ на своем проекте учебного симулятора рентгенэндоваскулярной хирургии":

                                                                                                                          V550 An odd precise comparison: t != 0. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. objectextractpart.cpp 3401

                                                                                                                          D3DXVECTOR3 N = VectorMultiplication(
                                                                                                                                            VectorMultiplication(V-VP, VN), VN);
                                                                                                                          float t = Qsqrt(Scalar(N, N));
                                                                                                                          if (t!=0)
                                                                                                                          {
                                                                                                                            N/=t;
                                                                                                                            V = V - N * DistPointToSurface(V, VP, N);
                                                                                                                          }
                                                                                                                          
                                                                                                                          Подобные ошибки повторяются достаточно часто в данной библиотеке. Не скажу, что это стало для меня неожиданностью. Уже ранее наталкивался на некорректную работу с числами с плавающей точкой в этом проекте. Однако систематически проверять исходники на этот счет не было ресурсов. По результатам проверки стало ясно, что нужно дать разработчику почитать что-то для расширения кругозора в части работы с числами с плавающей точкой. Скинул ему ссылки на пару хороших статей. Посмотрим на результат. Сложно однозначно сказать, вызывает ли эта ошибка реальные сбои в работе программы. Текущее решение выставляет ряд требований к исходной полигональной сетке артерий, по которым моделируется растекание рентгеноконтрастного вещества. Если требования не выполнены, то возможны падения программы или явно некорректная работа. Часть из этих требований получена аналитически, а часть эмпирически. Не исключено, что эта вторая часть требований растет как раз из некорректной работы с числами с плавающей точкой. Нужно отметить, что не все найденные случаи употребления точного сравнения чисел с плавающей точкой являлись ошибкой.

                                                                                                                          P.S. При желании уровень отдельных предупреждений в PVS-Studio можно изменить. Т.е. можно сделать V550 уровнем High.
                                                                                                                            0
                                                                                                                            > Я не помню ни единого случая из своей программистской практики, когда написанное программистом doubleValue == 0.0 приводило к багам, а abs(doubleValue) < someEpsilon исправляло эти баги.

                                                                                                                            В вычислительной математике — таки очень часто.
                                                                                                                            Хотя там нужно откровенно, конечно, ляпнуть, чтобы сравнивать с нулём — это уже ошибка уровня студента младших курсов. Видимо, поэтому и редко на практике — если уже пишется код, то такие вещи вбиты «в ДНК».
                                                                                                                            +1
                                                                                                                            V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression

                                                                                                                            Вы же всё время хвастаетесь, что концентрируетесь на реальных багах, а не на стиле кодирования? А это чисто стилистическое. Некоторым людям трудно представить булево значение в контексте любой операции кроме &&, ||,!.. Ну просто мозг не поворачивается в эту сторону. Поэтому они пишут длинно.

                                                                                                                              +2
                                                                                                                              Да, это может быть не ошибкой. Но ошибки тяготеют к такому коду. :)
                                                                                                                              0
                                                                                                                              Было бы интересно проанализировать когда-то давным-давно уплывшие исходники ядра Win2000…
                                                                                                                                +2
                                                                                                                                Подсудное дело…
                                                                                                                                  0
                                                                                                                                  Почему? Вы же не украли их сами и не используете в коммерческом проекте, вы их только изучаете. Вроде бы изучение материалов не считается чем-то плохим независимо от их происхождения. Можно конечно сказать что публикуя результат изучения вы таким образом делаете рекламу статическому анализатору, было бы интересно увидеть какой-то внятный комментарий о том какие здесь есть проблемы.
                                                                                                                                    +1
                                                                                                                                    Это к юристам.
                                                                                                                                      +3
                                                                                                                                      Проанализировать-то может и легально, но вот выкладывать результат такового анализа мне кажется, ой как сложно, чтобы ничего не нарушить. Могут нагреть за публикацию даже пары строк кода.

                                                                                                                                      Да и вопрос — зачем? Показать что в таком огромном проекте туева хуча опечаток допущенных по глупости? Реклама статическому анализатору для разработчиков из Microsoft? Ну, может быть, только как-то сомнительно выглядит. Лучше все же как сейчас, на максимально широкую аудиторию работать.
                                                                                                                                        0
                                                                                                                                        Для изучения есть абсолютно легальный WRK.
                                                                                                                                          0
                                                                                                                                          Продукт, предназначенный только для университетов, содержащий только сорцы части ядра. Так что не уверен в его абсолютной легальности.
                                                                                                                                            0

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

                                                                                                                                        +1
                                                                                                                                        Сложновато. Лучше всего анализатор прицепить к стадии компиляции, насколько я знаю, а исходники 2000 полностью не компилятся. Вот NT4 люди компилили, там можно было бы посмотреть.
                                                                                                                                        +1
                                                                                                                                        Продолжаем исследование качества кода: По следам калькуляторов: Qalculate!

                                                                                                                                        Присоединяйтесь к обсуждению :-)
                                                                                                                                          0

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

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