На днях компания 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
Комментарии (146)
agarus
09.03.2019 22:02+5«В таких крупных компаниях, как Microsoft, Google, Amazon и других, работает много талантливых программистов, но...» не только они :)
Спасибо, Святослав, за оперативность — спасибо Микрософту за рабочую субботу? :)
Areso
09.03.2019 22:23+2очень маленькая программа на языке C++
тут потерялась табличка сарказм, или 35 KSLOC сегодня считается маленькой программой (в частности, калькулятором)?
SvyatoslavMC Автор
09.03.2019 22:30+9Возможно, этот калькулятор крупнее обычного, согласен. Но с C++ сталкиваюсь постоянно, 35 — небольшой проект.
GreyStrannik
09.03.2019 22:43-10Данная статья наглядно демонстрирует чрезмерный уровень проверки качества кода инструментом. Очевидно, что калькулятор за 25 лет существования был избавлен от пользовательски-значимых ошибок. При этом тестируемый анализатор кода показывает наличие множества десятистепенных ошибок, которые не нужно исправлять — их исправление не принесёт пользу пользователям, а значит лишь увеличит затраты на поддержку калькулятора.
Andrey2008
09.03.2019 23:18+12При этом тестируемый анализатор кода показывает наличие множества десятистепенных ошибок, которые не нужно исправлять — их исправление не принесёт пользу пользователям, а значит лишь увеличит затраты на поддержку калькулятора.
Всё верно. Почти наверняка эти ошибки несущественны или находятся в неиспользуемом коде. Однако, комментарий может быть неправильно трактован, поэтому я хочу добавить.
Неэффективность правки старых ошибок, не означает неэффективность методологии статического анализа в целом.
Смысл статического анализа, не в разовом поиске старых ошибок. Да, мы это делаем, но исключительно в целях демонстрации возможностей анализатора. С точки зрения методологии — это неэффективно.
Смысл статичного анализа в том, чтобы использовать его регулярно. И тогда ошибки будут находиться ещё на написании кода, а не в процессе отладки, тестерами, пользователями и т.д. Т.е. цена исправления ошибки снижается с десятки и сотни раз. Да, статический анализ не способен найти все виды дефектов и заменить другие виды поиска ошибок. Нужно использовать комплекс мер. Однако, если хотя бы половина опечаток и прочих ляпов исправляется сразу, это уже круто.
P.S. В PVS-Studio даже есть режим, когда он показывает ошибки только в новом и отредактированном коде. А всё старое, считается техническим долгом, с которым можно неспешно работать (или вообще не работать :).Cenzo
10.03.2019 06:30P.S. В PVS-Studio даже есть режим, когда он показывает ошибки только в новом и отредактированном коде. А всё старое, считается техническим долгом, с которым можно неспешно работать (или вообще не работать :).
В PVS под Linux тоже есть такая возможность?
VBKesha
10.03.2019 02:09+5Такие ошибки могут спокойно жить в ядре системы и в критических сервисах, точно также по 10-15 лет а потом бац и очередной WannaCry стучится в порт.
keydon2
10.03.2019 03:12-2А зачем вы написали кусок кода, в котором наличие ошибки ни на что не влияет?
Тут явно статическим анализом не отделаешься…
arthi7471
10.03.2019 09:54-9«был избавлен от пользовательски-значимых ошибок»
Тавышо! А ничего что калькулятор из метро приложений не умеет правильно считать? Я не прикалываюсь. Задайте ему непосильную задачу (2х2)*2 увидите много интересного.
alchemist666
10.03.2019 11:13+1Это есть и в старом калькуляторе, и было "объяснение, почему он считает по другому(типа не инженерный калькулятор), на инженерном виде все правильно считает.
Вроде задачка выглядела по другому (2+2)*2
alix_ginger
10.03.2019 14:322 + 2 * 2, без скобок. Должно получиться 6, если калькулятор знает о порядке действий, может получиться 8, если не знает
GomboTs
10.03.2019 19:51+4Это, конечно, флейм-баян, но калькулятору в винде не положено знать о порядке действий.
Он имитирует древние «бухгалтерские» кнопочные калькуляторы, которые не имели никакого буфера и при нажатии следующей операции выполняли предыдущую и использовали ее результат как первый операнд.
lany
12.03.2019 08:50+2Вот, к примеру, мы пишем 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-код. А статический анализатор играючи указал на ошибку в нашем коде. Исправление тривиальное, а вот чтобы сконструировать тест-кейс, где ошибка проявляется, я потратил около получаса. Стоило ли исправлять или дожидаться недовольного пользователя? Как считаете?
Nagg
09.03.2019 23:19+10V547 Expression 'm_resolvedName == L«en-US»' is always false.
Шах и мат:
(ничего не менял, склонировал, запустил, поставил бряку)
is always false говорите?SvyatoslavMC Автор
09.03.2019 23:28+1Вчера также отлаживал, не удалось войти в эту ветку.
Nagg
09.03.2019 23:34+2Ох какая непостоянная ветка, представьте себе, в C# тоже строки по ссылкам в первую очередь сравнивают :-))
gdt
10.03.2019 05:58В C# представьте себе все ref объекты сравниваются в первую очередь по ссылкам, а ещё есть такое понятие как интернирование строк.
DistortNeo
10.03.2019 12:38+1В C# для строк перегружен оператор `==`.
Скорее всего, тут приколы со string interning.
SvyatoslavMC Автор
09.03.2019 23:37+13Коллеги, исходный код уже изменён в репозитории! Будьте внимательны при игре в шахматы)
Delics
09.03.2019 23:39Потому что эта ситуация не «always false», а «undefined behaviour».
Сообщение PVS-Studio некорректно.Andrey2008
10.03.2019 00:33Ммм… Возможно, но прошу дать ссылку на стандарт или другой документ где это уточняется. Сравниваются на равенство два указателя на wchat_t.
yleo
10.03.2019 01:42+2С точки зрения C/C++ это «always false».
Однако, в «военное время» компилятор (или даже линкер) может объединять константы (в том числе константные строки), и вот тогда сравнение будет работать правильно. Причем подобное не редкость в embedded-проектах.
klvov
10.03.2019 03:23И такая же фигня и в 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"); } }
Что вот выведет?
sotnikdv
10.03.2019 09:49+3Формально это неопределенное поведение. Т.е. результат будет сильно варьироваться от JVM, т.к. вопрос в том, есть ли и будет ли работать string pool.
HotSpot скажет True по дефолту, если просто запустить этот код, но нужно понимать, что string pool это такой себе tunable cache.
Поэтому его работа описывается «may not be cached».tmaxx
10.03.2019 10:55+8Серьезно? Формально, одинаковые строковые литералы в Java всегда равны по ссылке (пункт 3.10.5 language spec).
Можете привести пример JVM или настроек, при которых код выше выведет «You should always use String.equals when you want to compare strings for equality»?
yogg-soggot
10.03.2019 21:49Я когда писал приложение на Андроид случайно сравнил одинаковые строки через ==
Они оказались неравны.
Хотя это были переменные разных объектов.
Так что дело либо в этом, либо в Google JVM
vsb
10.03.2019 15:09+4Цитата из 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-ов и полагаются на быстрое сравнение указателей для производительности) и это поведение никогда не будет изменено.
NeocortexLab
11.03.2019 01:20Я в статье-обзоре дал линк, так минусовать начали. За что — не понятно.
habr.com/ru/post/443018/#comment_19854498
Там написано же у M$ почему так работает
mynameco
10.03.2019 04:41Я как то читал давно. Что из за того что многие в функцию принт передавали std string и их вариант из mfc то ms просто поддержали и эти случаи, хотя это и ub. Возможно тут тоже компилятор заменяет ub на то что ожидает пользователь. Но это не точно.
Andrey2008
10.03.2019 10:30Это навреное была моя статья "Большой брат помогает тебе".
В данном случае никакого UB нет. Один указатель на wchar_t сравнивается с другим указателем. Вот и всё. А вся дискуссия пошла из-за того, что в заголовочном файле массив символов m_resolvedName уже превратился в полноценную строку типа std::wstring. И теперь сравнение работает правильно.
qw1
10.03.2019 12:07+2Потому что эта ситуация не «always false», а «undefined behaviour».
Никак нет, потому что в коде
wchar_t buffer[100] = L"test";
if (buffer == L"test")
компилятор не имеет права этим двум строкам дать один адрес, т.к. у них разная const-ность.DistortNeo
10.03.2019 12:45Почему это не имеет права? В современных компиляторах спецификатор
constиспользуется только для контроля доступа и выбора нужных перегрузок функций, после чего он игнорируется. Компилятор дальше сам определяет, возможна ли модификация области памяти или нет. И если модификация невозможна, то почему бы не сэкономить на памяти?
Delics
10.03.2019 13:03-2Ваш пример не сработает, т.к. выделяется отдельная память для массива.
А так:
wchar_t * buffer = L"test"; if (buffer == L"test") { long g = 0; }
Получается вот что:
Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу. Раз в сообщении выше показано, что точка останова сработала, значит как раз ссылка на константу и была передана в функцию.
Объединять ли одинаковые текстовые константы в одну область памяти или нет — это уже на усмотрение компилятора, а значит «undefined behaviour»qw1
10.03.2019 13:22+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";
qw1
10.03.2019 13:24+1Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу. Раз в сообщении выше показано, что точка останова сработала, значит как раз ссылка на константу и была передана в функцию
А посмотреть исходник? Благо даже скачивать не надо, github позволяет браузить репозиторий online. В оригинале был массив
wchar_t m_resolvedName[100];
Andrey2008
10.03.2019 16:11+4Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу.
Ох… Ну сколько можно… В статье чётко написано:
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. Уже несколько раз это обсудили в комментариях.
Delics
10.03.2019 18:19-6Ох… Ну сколько можно… В статье чётко написано:
В статье было написано о сравнении двух указателей и о том, что «такое сравнение всегда false». До того, как изменили код.
Это сообщение PVS-Studio неверно, что было доказано моим примером выше.Andrey2008
10.03.2019 18:31+2Вы осознаёте, что
принципиально разные сущности в C++?const wchar_t *A = L"test"; wchar_t B[] = L"test";Delics
10.03.2019 18:41-10В первоначальной редакции статьи никакого указания, что передаётся массив, не было. Поменяли, теперь стало иначе.
Пока очевидно, что скилл PVS Studio недостаточен, чтобы учить других программистов. Разработчики просто не знали про Пул строк.Andrey2008
10.03.2019 19:04+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. Только не говорите, что и ссылки на документацию тоже не было :).Delics
10.03.2019 19:12-71. Разницу между строковой константой и массивом я знаю
2. С массивом тоже будет «undefined behaviour», а не «always false».
Посмотрел, что пишет в такой ситуации профессиональная система анализа кода. JerBrains CLion:
Просто «unspecified». И предложение воспользоваться strncmp. Всё, никаких «always false»
Это признаём.
Да уж, я заметил, как вы «признаёте». Признавали бы, этот диалог мог сложиться иначе.qw1
10.03.2019 19:39+32. С массивом тоже будет «undefined behaviour», а не «always false».
Объясните, почему?
Массив — член класса его адрес считается как смещение поля от адреса объекта. Строковый литерал — статическое значение. Не важно, размещено в пуле или ещё где-то в секции read-only data. Почему адреса могут совпасть? Разве в стандарте запрещено сравнение указателя с литералом? Вероятно, тут CLion не прав.Delics
10.03.2019 20:56-4Объясните, почему?
Ленивые присваивания строк.
В MSVC не применяется, но, теоретически, такая оптимизация возможна.qw1
10.03.2019 22:34+2Ленивые присваивания строк.
Это касается только классов-обёрток (типа std::string), но никак не Си-шных «строк» (char*)Delics
10.03.2019 23:21-3Это касается только классов-обёрток (типа std::string)
Речь здесь и ранее идёт про строковые константы. Когда строка попадает в std::string она уже не может быть добавлена в пул.
В целом, поздравляю, у и вас и у PVS Studio сегодня level-up. Столько всего нового узнали.
Возможно, в следующей версии статьи, там вообще не будет ошибок.
creker
10.03.2019 18:20-1Скорее всего не понадобится ни везение, ни фазы луны. VS использует string pooling при настройках по-умолчанию что в debug, что в release сборке.
creker
10.03.2019 18:36-1Тут речь о том, что сообщение PVS-Studio некорректное или неполное, что привело к неправильной интерпретации этого сообщения даже у вас в статье. Сравнение не будет всегда false для сравнения указателей(!), это показывает практика string pooling в VS. В данном конкретном случае проблема не в сравнении указателей, а в том, что m_resolvedName это массив и в С++ нужно очень сильно извратиться, чтобы условие сработало, если это вообще возможно. В данном случае PVS-Studio должна была написать именно это — переменная массив и условие потенциально никогда не выполнится.
А вот если бы m_resolvedName было простым wchar_t*, то условие очень даже сработает, если есть string pooling или аналог в данном конкретном компиляторе. В этом случае PVS-Studio стоит предупредить, что это потенциально непереносимый код, который зависит от конфигурации и типа компилятора. Работать будет, но лучше исправить.Andrey2008
10.03.2019 18:49В сообщении сложно уместить описание проблемы. Однако, этот вопрос рассматривается в документации к диагностике V547. А в статье просто не аккуратно написано, потому что очень торопились. Перед переводом подретушируем. Не хватает вокруг этого случая ещё и дискуссии на англоязычных форумах устраивать… :)
creker
10.03.2019 18:56«m_resolvedName is an array» очень легко уместить и это, я думаю, сняло бы вопросы и утверждение «always false» было бы действительно верно. Опять же, будь переменная простым указателем, диагностика была бы вообще некорректная от слова совсем. В документации упоминается пулинг строк, но не упоминается ситуация с массивами, которые в С++ ведут себя совсем иначе, нежели в С.
Как я понял, товарищ Delics именно это и хотел донести, но его почему-то минусуют.qw1
10.03.2019 19:34В данном случае 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 с длиной, это не замена оператору==).creker
10.03.2019 21:20Может, для кого-то и надо так разжёвывать, но как по мне «expression is always false» тут самое лучшее
Тут оно действительно лучшее, если бы второй кейс имел другое описание. С моей стороны была догадка, но, получается, она сбылась и какое-то из диагностических сообщений надо исправлять. Проще всего конечно «always false» заменить на UB и ничего не надо объяснять никому.
Miron11
11.03.2019 13:50Не комплексуйте. Устраивайте дискуссии.
Их на самом деле не… бурили достаточно. Очень похожую ошибку как — то нашел у них в документации. Они её поправили, а мне в ответ пишут «Ваше замечание бессмысленно». Моя попытка напомнить публике, что надо сказать «спасибо» натолкнулась на корпоративное «фи», мою учетную запись забанили.
Так что понаглее, локти по-шире и коленкой вперед! :)))))))))))
Удачи.
KumoKairo
09.03.2019 23:54+4Обратите внимание на сигнатуры в вашем коде —
wstringпротивwchar_tиз статьи.wstringподдерживает структурное равенство через==, массивwchar_t— нет.
Именно такие малозаметные изменения и опечатки говорят в пользу статического анализа кода.Nagg
10.03.2019 00:03Ставлю сотку приложение спрототипировали на C# и потом переписали на С++ CLI/CX, отсюда таки перлы, благо незаход в ветку никак не ломал приложение — это была лишь оптимизация насколько я понял.
qw1
10.03.2019 12:11Ставлю сотку приложение спрототипировали на C# и потом переписали на С++ CLI/CX
Но зачем?
mk2
10.03.2019 07:25А вот коммит, который это поменял — github.com/Microsoft/calculator/commit/c85d7ec4549ff06a7b7dc08201c671336725fba8
И сделано это буквально вчера.
EvgeniyRyzhkov
10.03.2019 07:28+3Хороший ответ на «эти ошибки никому не мешают». Человек не поленился в выходной поправить ошибку в рабочем проекте.
unclejocker
10.03.2019 10:37+1Ну стыдно же:) Если в вашем проекте такое найдут и в новостях напишут, я думаю вы тоже подорветесь править.
qw1
10.03.2019 11:58+2Мне только интересно, эти исправления сделаны на публику в гитхабе, или они попали во внутренние репозитории MS, из которых собирается продукт. Узнаем со следующим апдейтом, будет ли туда включен calc.exe.
farcaller
10.03.2019 00:23+1А перевод этой статьи на английский есть? Порылся в вашем блоге, но не нашел.
Andrey2008
10.03.2019 00:26Ещё нет, но будет. Решили опубликовать пока идёт бурное обсуждение новости, не дожидаясь перевода.
SvyatoslavMC Автор
10.03.2019 00:26+2Я опубликовал свеженаписаннаю статью. Перевод подъедет чуть позже.
dimm_ddr
10.03.2019 13:56Планируете еще и на хабр перевод выложить? С открытием англоязычного хабра это имеет смысл.
SvyatoslavMC Автор
12.03.2019 12:13+1Подготовили перевод:
Следите за новостями. Скоро подъедут сравнения с другими популярными калькуляторами)
WinPooh73
10.03.2019 09:26Интересно было бы теперь для сравнения проверить код юниксового калькулятора bc.
EvgeniyRyzhkov
10.03.2019 10:21+6Вы можете это сделать абсолютно самостоятельно, чтобы была статья от независимого автора. Так будет даже интереснее! И PVS-Studio доступна, и исходники калькулятора есть. Поможете нам сделать серию статей о проверке калькуляторов? ;-)
WinPooh73
10.03.2019 10:31+2Спасибо за идею, обязательно попробую. Заодно освою новый для себя инструмент. Честно говоря, не знал о существовании триальной версии PVS :)
MaximRV
10.03.2019 10:12-7Прикольно. В MS это писали «студенты». Как повысить качество кода? Выложить в открытый доступ. Тут же набросились профессионалы, и вуаля — код улучшился. Красиво и дёшево.
qw1
10.03.2019 12:02Ну нашли 100500 банальных опечаток. Бизнес-пользы в этом никакой, разве что платить дополнительно сотрудникам, которые промониторят найденные ошибки и внесут исправления в репозиторий.
Другое дело, если бы кто-то нашёл архитектурные ошибки и глобально отрефакторил — тогда бы код улучшился и его дешевле было бы развивать в будущем. Но что-то таких героев не видно.
xPomaHx
10.03.2019 10:26+4Главная мысль это то что для коммерческого успеха идеальный код не нужен. Это на заметку собеседющим, главное это человеческие качества, имхо основное это умение довести до конца.
mapron
10.03.2019 19:12+1Ну и приложение из стандартной поставки Windows это тоже не «коммерческий успех», прошу заметить — кто его самостоятельно ставить-то или покупать будет? Так что мысль хорошая, только далековата от предмета статьи =)
dimm_ddr
11.03.2019 09:52Стандартная поставка влияет на коммерческий успех Windows. Другое дело что калькулятор вряд ли вносит хоть сколько-то заметный вклад в этот успех, поэтому практически вы правы.
neurocore
10.03.2019 10:56+1А что это за интересный синтаксис:
String^Andrey2008
10.03.2019 10:58+3Это C++/CLI.
neurocore
10.03.2019 11:03+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.)
DanilinS
10.03.2019 11:41-13Докатились. Калькулятор не можем написать без кучи багов и неоднозначного кода.
А сразу правильно что не пишется? Благо логики в калькулаторе минимум.
gaploid
10.03.2019 12:14Похоже, калькулятор это первые шаги перед переводом чего-то большого в опенсорс. Так же было вначале с .net, сначала библиотеки, unit тесты и тд.
DjSens
10.03.2019 13:26+1у меня комп флешку не отдаёт пока калькулятор не закроешь в некоторых случаях (похоже когда в тотал коммандере открыта флешка и я запускаю с панели тотала иконку калькулятора)
unC0Rr
10.03.2019 14:34+7Видимо, тотал ставит калькулятору флешку в качестве текущей директории, отчего она становится занятой.
sergpank
10.03.2019 19:44+4Поэтому я когда-то забросил С++ и перешел на Java. Ибо слишком глуп и невнимателен для плюсов. Легкость с которой можно сделать ошибку сравнима с игрой в super meat boy на хардкорных настройках. Особенно когда ты пытаешься вклинить свой код в огромный проект, написанный такими же раздолбаями как и ты сам.
Часто возникали ситуации «Мой код не работает — ПОЧЕМУ?!» и «Мой код работает — ПОЧЕМУ?!». И чем больше я изучал плюсы тем активнее искал из них выход.
Все-таки хороший плюсовик очень редкая птица. Их нужно ценить, беречь и давать молоко за вредность.staticlab
10.03.2019 23:11Все-таки хороший плюсовик очень редкая птица. Их нужно ценить, беречь и давать молоко за вредность.
Интересно, что некоторые товарищи с Хабра, ищущие плюсовиков, считают как раз наоборот :)
Jogger
10.03.2019 20:29+3Это не тот калькулятор который бы мне хотелось увидеть. Последний раз нормальный калькулятор был в XP кажется. А в этом так удобно — надо тебе взять синус из числа 5Eh, ты сначала переключить в режим для программистов, конвертируй число, ЗАПОМНИ ЕГО, перейди в режим научного калькулятора (при этом число в калькуляторе очистится), введи число заново, и посчитай синус. То есть вместо двух кликов — куча ненужных действий, включая ползание по менюшкам. Какая разница, сколько в нём ошибок, если им пользоваться неудобно? И самое обидное — раньше хоть на сайте микрософта можно было скачать так называемый Calculator Plus, который точная копия того самого старого калькулятора, только зачем-то с дополнительным скином (чудовищно выглядящим и установленным по умолчанию). А теперь и его удалили. Благо хоть инсталляха сохранилась. И пусть молодёжь смеётся что хранить хлам на винте прошлый век, всё же можно скачать.
lany
12.03.2019 08:38+1Вот вам сорцы того самого калькулятора!
Jogger
12.03.2019 11:31Спасибо. Было бы интересно проверить и его (есть подозрение что с ним ситуация будет ещё хуже), но для этого хорошо бы знать как его правильно компилить. Я конечно засунул его в визуал студию, и даже смог собрать что-то относительно работоспособное, но не до конца понятно где косяки исходного кода а где — мои кривые руки.
lany
12.03.2019 11:46Ну это была утечка какого-то куска Windows 2000. Никто не гарантировал, что она полная и работоспособная.
AndrewN
11.03.2019 05:21+1Анализатор обнаружил две функции, которые реализованы одинаково. По названиям функций Convert() и ConvertBack() можно предположить, что они должны выполнять разные действия, но разработчикам виднее.
Здесь как раз все верно. Если у нас конвертер инвертирует булево значение, то очевидно, что операция обратная инвертированию является инвертированием :) Т.е. x == !!x.
SvyatoslavMC Автор
11.03.2019 07:48Эта диагностика выяляет 2 проблемы:
- Функции имеют одинаковую реализацию (не всегда ошибка);
- Дублирование кода само по себе плохо. Проблемы начинаются, когда необходимо изменять код.
AndrewN
11.03.2019 08:152. Плохо, но в этом конкретном случае я бы выносить это в третью функцию не стал. Впрочем, анализатор, конечно, все кейсы различать не научишь.
FloorZ
11.03.2019 21:05Мне кажется, что это был задел на будущее.
Сделали две одинаковые функции, на случай, если ConvertBack приобретет новую реализацию. Или возможно в планах было сделать другую реализацию. Но руки не дошли.qw1
11.03.2019 21:10Как писавший немало конвертеров для XAML, скажу что тут нет никакого задела. BooleanNegationConverter — полностью законченная и самостоятельная вешь, нужная, если хочешь забиндить состояние чекбокса на булевскую переменную, но не на её значение, а на инверсию. Просто биндишь через этот конвертор, и всё. А что функция конверсии получается такая длинная (вместо одной строки bool result = !input), это издержки технологии, открывающие многие дополнительные возможности, когда они будут нужны.
Artemis86
11.03.2019 07:40Спасибо за статью, всегда читаю с интересом. Ребята, вы понимаете что Роскосмос, NASA и прочие должны уже в очереди стоять за вашими разработками? Кто знает сколько спутников потерялось из-за таких мелких ошибок?
WayMax
11.03.2019 12:15Немного не по теме, но планируется ли в PVS-Studio поддержка PHP?
SvyatoslavMC Автор
11.03.2019 12:20Пока нет. Если решим поддержать новый язык, то проведём опросник. Следите за нашим блогом)
Artemis86
11.03.2019 16:58А вообще планируете какие-нибудь языки добавлять, если не секрет конечно? И какие, если да?
SvyatoslavMC Автор
11.03.2019 17:013 месяца назад добавили анализатор Java. Некоторое время мы будем заниматься развитием и поддержкой существующих языков, прежде чем выберем новый.
FloorZ
11.03.2019 21:18-1У меня есть предположение по эксепшинам.
Возможно std:: exception был специально запривачен, что бы исключения нельзя было перехватит через catch( std::exception&).
М.б. они они хотели калькулятор в виде либы сделать.
А их реализация запривачена, без dll import.
Ошибки с условиями очень странные.
Где (а && б ||! а &&! б)…
Возможно ранее, эти переменные были числами. И им нужно было проверить, что они равны или не равны нулям. А между собой, равенства не имеют. От чего, сравнение a == b не возможно.
По поводу указателей.
Тут наверное надо документацию по clr прочесть надо.
М.б. для wchar_t и литерала L там прописаны свои правила компиляции операторов сравненияSvyatoslavMC Автор
11.03.2019 21:26+2Да не было тут никакой предыстории. Просто люди ошибаются. Как выявили эти проблемы — сразу исправили. Применение статического анализа должно улучшить ситуацию.
Andrey2008
11.03.2019 21:40+1По количеству просмотров и комментариев чувствуется, что калькуляторы волнуют людей :). И автор этой статьи уже пишет заметку про калькулятор Qalculate! и делает facepalm-ы. Причина скоро станет понятна. Если совсем кратко: там всё хуже, чем в калькуляторе от Microsoft.
Но я вот к чему. Не обязательно ждать, когда мы проверим тот или иной проект. Есть различные варианты бесплатного использования PVS-Studio. Плюс можно триальную версию использовать. Можно и что-то проверить и статью написать. Или ошибки в любом проекте поправить. Подробнее: Бесплатные варианты лицензирования PVS-Studio.dev96
11.03.2019 23:26А насколько скоро статья будет?
Кстати, по теме бесплатной версии: раньше были ограниченные переходы в бесплатной версии. Потом, в какой-то момент после переустановки системы, я поставил PVS и ограничение переходов вообще отсутствовало. Это был баг или вы ограничение убрали?Andrey2008
11.03.2019 23:37Ограничение переходов (кликов) относилось к демо-версии. Видимо она и была у Вас установлена. А в бесплатных версиях никакого ограничения на клики не было и нет.
dev96
11.03.2019 23:43Так я не выполнял никакие действия из бесплатной лицензии, так что обычная демо и была.
lany
12.03.2019 08:32Да, калькулятор сильно людям понравился. Вроде обычная статья от 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. Скажите, вы ругаетесь на операцию>=? Ведь здесь фактически она и есть.Andrey2008
12.03.2019 09:16Да, 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.
lany
12.03.2019 08:35V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression
Вы же всё время хвастаетесь, что концентрируетесь на реальных багах, а не на стиле кодирования? А это чисто стилистическое. Некоторым людям трудно представить булево значение в контексте любой операции кроме &&, ||,!.. Ну просто мозг не поворачивается в эту сторону. Поэтому они пишут длинно.
Al_Azif
12.03.2019 14:32Было бы интересно проанализировать когда-то давным-давно уплывшие исходники ядра Win2000…
EvgeniyRyzhkov
12.03.2019 14:37Подсудное дело…
dimm_ddr
12.03.2019 14:57+1Почему? Вы же не украли их сами и не используете в коммерческом проекте, вы их только изучаете. Вроде бы изучение материалов не считается чем-то плохим независимо от их происхождения. Можно конечно сказать что публикуя результат изучения вы таким образом делаете рекламу статическому анализатору, было бы интересно увидеть какой-то внятный комментарий о том какие здесь есть проблемы.
elmm
Сравнение строк указателей, не освобождённый буфер — такие ошибки выглядят так, как буд-то над кодом местами поработал студент-практикант.
ooprizrakoo
а так вполне возможно и было — в MS много интернов, которым отдают в разработку некритичные части софта.
qwertyqwerty
Юниоры писали.
staticlab
Очень может быть, что писали шарписты, которых заставили реюзать плюсовый код и потому писать на C++/CX. Отсюда, в частности, и ошибки с приватным наследованием, и итерирование энума.
maxzhurkin
Скорее всего, бОльшая часть кода написана ещё до появления C#, всё же
staticlab
Код ядра калькулятора тянется ещё с 1989 года, если верить исходникам.
arctic-fox
Хочу видеть историю их гита с 1989 года. Что пушилось, что мерджилось, где в форки ушло, и в какие форки. Когда происходил ремастер, т.к. в 10 калькулятор довольно сильно отличается от 95.
staticlab
Гит появился только в 2005 :) В любом случае вряд ли когда-либо MS выложит полную историю правок исходников, даже если она у них и осталась.
arctic-fox
К сожалению — да, но даже с 2005 года будет интересно.
Что не понятно, почему минусов за желание увидеть, пусть и синтезированную гит историю насыпалось.