Комментарии 166
Спасибо, Святослав, за оперативность — спасибо Микрософту за рабочую субботу? :)
очень маленькая программа на языке C++
тут потерялась табличка сарказм, или 35 KSLOC сегодня считается маленькой программой (в частности, калькулятором)?
Насколько я понял, это калькулятор из Windows 10, не думаю, что ему 25 лет
Очевидно, что калькулятор за 25 лет существования был избавлен от пользовательски-значимых ошибок.
откуда уверенность, что в выложенном куркуляторе много кода из предыдущих версий?
При этом тестируемый анализатор кода показывает наличие множества десятистепенных ошибок, которые не нужно исправлять — их исправление не принесёт пользу пользователям, а значит лишь увеличит затраты на поддержку калькулятора.
Всё верно. Почти наверняка эти ошибки несущественны или находятся в неиспользуемом коде. Однако, комментарий может быть неправильно трактован, поэтому я хочу добавить.
Неэффективность правки старых ошибок, не означает неэффективность методологии статического анализа в целом.
Смысл статического анализа, не в разовом поиске старых ошибок. Да, мы это делаем, но исключительно в целях демонстрации возможностей анализатора. С точки зрения методологии — это неэффективно.
Смысл статичного анализа в том, чтобы использовать его регулярно. И тогда ошибки будут находиться ещё на написании кода, а не в процессе отладки, тестерами, пользователями и т.д. Т.е. цена исправления ошибки снижается с десятки и сотни раз. Да, статический анализ не способен найти все виды дефектов и заменить другие виды поиска ошибок. Нужно использовать комплекс мер. Однако, если хотя бы половина опечаток и прочих ляпов исправляется сразу, это уже круто.
P.S. В PVS-Studio даже есть режим, когда он показывает ошибки только в новом и отредактированном коде. А всё старое, считается техническим долгом, с которым можно неспешно работать (или вообще не работать :).
P.S. В PVS-Studio даже есть режим, когда он показывает ошибки только в новом и отредактированном коде. А всё старое, считается техническим долгом, с которым можно неспешно работать (или вообще не работать :).
В PVS под Linux тоже есть такая возможность?
Тут явно статическим анализом не отделаешься…
Тавышо! А ничего что калькулятор из метро приложений не умеет правильно считать? Я не прикалываюсь. Задайте ему непосильную задачу (2х2)*2 увидите много интересного.
Это есть и в старом калькуляторе, и было "объяснение, почему он считает по другому(типа не инженерный калькулятор), на инженерном виде все правильно считает.
Вроде задачка выглядела по другому (2+2)*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-код. А статический анализатор играючи указал на ошибку в нашем коде. Исправление тривиальное, а вот чтобы сконструировать тест-кейс, где ошибка проявляется, я потратил около получаса. Стоило ли исправлять или дожидаться недовольного пользователя? Как считаете?
V547 Expression 'm_resolvedName == L«en-US»' is always false.
Шах и мат:
(ничего не менял, склонировал, запустил, поставил бряку)
is always false говорите?
Однако, в «военное время» компилятор (или даже линкер) может объединять константы (в том числе константные строки), и вот тогда сравнение будет работать правильно. Причем подобное не редкость в embedded-проектах.
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");
}
}
Что вот выведет?
Серьезно? Формально, одинаковые строковые литералы в Java всегда равны по ссылке (пункт 3.10.5 language spec).
Можете привести пример JVM или настроек, при которых код выше выведет «You should always use String.equals when you want to compare strings for equality»?
String::equals уже по умолчанию использует сравнение по указателю для производительности, как в Oracle JDK, так и в Open JDK и «даже» в Android JDK. Если сорить такими сравнениями ещё и за пределами equals, в собственном коде, то статистически максимальный выигрыш будет стремиться к стоимости вызова единственного метода. Стоит ли овчинка выделки с учётом всей машинерии внутри — вопрос более чем открытый.На всякий случай, всё-таки поясню почему вы не правы.
Если компилятору разрешить объединять константы (см ниже), то он вправе совместить все байты всех констант, с учетом требований к выравниванию их типов. Более того, аналогичный функционал может быть параллельно или независимо реализован в линкере.
В рассматриваемом случае такое объединение точно произойдет, если где-то в пределах видимости компилятора (или "умного" линкера) есть константа с совпадающим названием локали.
В свою очередь, если 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.
Я как то читал давно. Что из за того что многие в функцию принт передавали std string и их вариант из mfc то ms просто поддержали и эти случаи, хотя это и ub. Возможно тут тоже компилятор заменяет ub на то что ожидает пользователь. Но это не точно.
В данном случае никакого UB нет. Один указатель на wchar_t сравнивается с другим указателем. Вот и всё. А вся дискуссия пошла из-за того, что в заголовочном файле массив символов m_resolvedName уже превратился в полноценную строку типа std::wstring. И теперь сравнение работает правильно.
Потому что эта ситуация не «always false», а «undefined behaviour».Никак нет, потому что в коде
wchar_t buffer[100] = L"test";
if (buffer == L"test")компилятор не имеет права этим двум строкам дать один адрес, т.к. у них разная const-ность.
Почему это не имеет права? В современных компиляторах спецификатор const используется только для контроля доступа и выбора нужных перегрузок функций, после чего он игнорируется. Компилятор дальше сам определяет, возможна ли модификация области памяти или нет. И если модификация невозможна, то почему бы не сэкономить на памяти?
Так можно:
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";Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу. Раз в сообщении выше показано, что точка останова сработала, значит как раз ссылка на константу и была передана в функциюА посмотреть исходник? Благо даже скачивать не надо, github позволяет браузить репозиторий online. В оригинале был массив
wchar_t m_resolvedName[100];Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу.
Ох… Ну сколько можно… В статье чётко написано:
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. Уже несколько раз это обсудили в комментариях.
const wchar_t *A = L"test";
wchar_t B[] = L"test";
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. Только не говорите, что и ссылки на документацию тоже не было :).
2. С массивом тоже будет «undefined behaviour», а не «always false».Объясните, почему?
Массив — член класса его адрес считается как смещение поля от адреса объекта. Строковый литерал — статическое значение. Не важно, размещено в пуле или ещё где-то в секции read-only data. Почему адреса могут совпасть? Разве в стандарте запрещено сравнение указателя с литералом? Вероятно, тут CLion не прав.
А вот если бы m_resolvedName было простым wchar_t*, то условие очень даже сработает, если есть string pooling или аналог в данном конкретном компиляторе. В этом случае PVS-Studio стоит предупредить, что это потенциально непереносимый код, который зависит от конфигурации и типа компилятора. Работать будет, но лучше исправить.
Как я понял, товарищ Delics именно это и хотел донести, но его почему-то минусуют.
В данном случае 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 с длиной, это не замена оператору
== ).Может, для кого-то и надо так разжёвывать, но как по мне «expression is always false» тут самое лучшее
Тут оно действительно лучшее, если бы второй кейс имел другое описание. С моей стороны была догадка, но, получается, она сбылась и какое-то из диагностических сообщений надо исправлять. Проще всего конечно «always false» заменить на UB и ничего не надо объяснять никому.
Их на самом деле не… бурили достаточно. Очень похожую ошибку как — то нашел у них в документации. Они её поправили, а мне в ответ пишут «Ваше замечание бессмысленно». Моя попытка напомнить публике, что надо сказать «спасибо» натолкнулась на корпоративное «фи», мою учетную запись забанили.
Так что понаглее, локти по-шире и коленкой вперед! :)))))))))))
Удачи.
wstring против wchar_t из статьи. wstring поддерживает структурное равенство через ==, массив wchar_t — нет. Именно такие малозаметные изменения и опечатки говорят в пользу статического анализа кода.
И сделано это буквально вчера.
в выходной
В США 8 марта выходным не является. Поэтому все же в обычный рабочий день
А перевод этой статьи на английский есть? Порылся в вашем блоге, но не нашел.
Следите за новостями. Скоро подъедут сравнения с другими популярными калькуляторами)
Интересно было бы теперь для сравнения проверить код юниксового калькулятора bc.
Другое дело, если бы кто-то нашёл архитектурные ошибки и глобально отрефакторил — тогда бы код улучшился и его дешевле было бы развивать в будущем. Но что-то таких героев не видно.
String^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.)
А сразу правильно что не пишется? Благо логики в калькулаторе минимум.
у меня комп флешку не отдаёт пока калькулятор не закроешь в некоторых случаях (похоже когда в тотал коммандере открыта флешка и я запускаю с панели тотала иконку калькулятора)
Часто возникали ситуации «Мой код не работает — ПОЧЕМУ?!» и «Мой код работает — ПОЧЕМУ?!». И чем больше я изучал плюсы тем активнее искал из них выход.
Все-таки хороший плюсовик очень редкая птица. Их нужно ценить, беречь и давать молоко за вредность.
Вот вам сорцы того самого калькулятора!
Анализатор обнаружил две функции, которые реализованы одинаково. По названиям функций Convert() и ConvertBack() можно предположить, что они должны выполнять разные действия, но разработчикам виднее.
Здесь как раз все верно. Если у нас конвертер инвертирует булево значение, то очевидно, что операция обратная инвертированию является инвертированием :) Т.е. x == !!x.
- Функции имеют одинаковую реализацию (не всегда ошибка);
- Дублирование кода само по себе плохо. Проблемы начинаются, когда необходимо изменять код.
Мне кажется, что это был задел на будущее.
Сделали две одинаковые функции, на случай, если ConvertBack приобретет новую реализацию. Или возможно в планах было сделать другую реализацию. Но руки не дошли.
У меня есть предположение по эксепшинам.
Возможно std:: exception был специально запривачен, что бы исключения нельзя было перехватит через catch( std::exception&).
М.б. они они хотели калькулятор в виде либы сделать.
А их реализация запривачена, без dll import.
Ошибки с условиями очень странные.
Где (а && б ||! а &&! б)…
Возможно ранее, эти переменные были числами. И им нужно было проверить, что они равны или не равны нулям. А между собой, равенства не имеют. От чего, сравнение a == b не возможно.
По поводу указателей.
Тут наверное надо документацию по clr прочесть надо.
М.б. для wchar_t и литерала L там прописаны свои правила компиляции операторов сравнения
Но я вот к чему. Не обязательно ждать, когда мы проверим тот или иной проект. Есть различные варианты бесплатного использования PVS-Studio. Плюс можно триальную версию использовать. Можно и что-то проверить и статью написать. Или ошибки в любом проекте поправить. Подробнее: Бесплатные варианты лицензирования PVS-Studio.
Да, калькулятор сильно людям понравился. Вроде обычная статья от 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. Скажите, вы ругаетесь на операцию >=? Ведь здесь фактически она и есть.
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.
В вычислительной математике — таки очень часто.
Хотя там нужно откровенно, конечно, ляпнуть, чтобы сравнивать с нулём — это уже ошибка уровня студента младших курсов. Видимо, поэтому и редко на практике — если уже пишется код, то такие вещи вбиты «в ДНК».
V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression
Вы же всё время хвастаетесь, что концентрируетесь на реальных багах, а не на стиле кодирования? А это чисто стилистическое. Некоторым людям трудно представить булево значение в контексте любой операции кроме &&, ||,!.. Ну просто мозг не поворачивается в эту сторону. Поэтому они пишут длинно.
Да и вопрос — зачем? Показать что в таком огромном проекте туева хуча опечаток допущенных по глупости? Реклама статическому анализатору для разработчиков из Microsoft? Ну, может быть, только как-то сомнительно выглядит. Лучше все же как сейчас, на максимально широкую аудиторию работать.
Присоединяйтесь к обсуждению :-)
Нашёл баг в калькуляторе windows 7.
Переключаемся в инженерный режим.
Набирает 1/(17*18)= и нажимаем кнопку [F-E], это вывод в экспоненциальной форме.
И калькулятор аварийно падает, переполнение в стеке. В 10-ке всё нормально, такого бага нет. В ХР проверю чуть позже... Проверил, всё нормально, бага нет.
Подсчитаем баги в калькуляторе Windows