Как стать автором
Поиск
Написать публикацию
Обновить

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

Сравнение строк указателей, не освобождённый буфер — такие ошибки выглядят так, как буд-то над кодом местами поработал студент-практикант.
Всего голосов 30: ↑27 и ↓3+24
а так вполне возможно и было — в MS много интернов, которым отдают в разработку некритичные части софта.
Всего голосов 16: ↑16 и ↓0+16
Юниоры писали.
Всего голосов 3: ↑3 и ↓0+3
НЛО прилетело и опубликовало эту надпись здесь
Скорее всего, бОльшая часть кода написана ещё до появления C#, всё же
Комментарий пока не оценивали0
НЛО прилетело и опубликовало эту надпись здесь
Хочу видеть историю их гита с 1989 года. Что пушилось, что мерджилось, где в форки ушло, и в какие форки. Когда происходил ремастер, т.к. в 10 калькулятор довольно сильно отличается от 95.
Всего голосов 7: ↑3 и ↓4-1
НЛО прилетело и опубликовало эту надпись здесь
К сожалению — да, но даже с 2005 года будет интересно.
Что не понятно, почему минусов за желание увидеть, пусть и синтезированную гит историю насыпалось.
Всего голосов 1: ↑1 и ↓0+1

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

Всего голосов 1: ↑1 и ↓0+1
НЛО прилетело и опубликовало эту надпись здесь
Ну глядя на калк в последней винде, в этом можно быть уверенным на 100%. По крайней мере в части, отвечающей за интерфейс…
Всего голосов 1: ↑1 и ↓0+1
НЛО прилетело и опубликовало эту надпись здесь
«В таких крупных компаниях, как Microsoft, Google, Amazon и других, работает много талантливых программистов, но...» не только они :)
Спасибо, Святослав, за оперативность — спасибо Микрософту за рабочую субботу? :)
Всего голосов 13: ↑12 и ↓1+11
Я ещё в пятницу начал)
Всего голосов 8: ↑8 и ↓0+8
НЛО прилетело и опубликовало эту надпись здесь
очень маленькая программа на языке C++

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

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

Всего голосов 21: ↑20 и ↓1+19
Очевидно, что калькулятор за 25 лет существования был избавлен от пользовательски-значимых ошибок.

откуда уверенность, что в выложенном куркуляторе много кода из предыдущих версий?
Всего голосов 4: ↑4 и ↓0+4
НЛО прилетело и опубликовало эту надпись здесь
При этом тестируемый анализатор кода показывает наличие множества десятистепенных ошибок, которые не нужно исправлять — их исправление не принесёт пользу пользователям, а значит лишь увеличит затраты на поддержку калькулятора.

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

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

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

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

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

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

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

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

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

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

Вот, к примеру, мы пишем 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-код. А статический анализатор играючи указал на ошибку в нашем коде. Исправление тривиальное, а вот чтобы сконструировать тест-кейс, где ошибка проявляется, я потратил около получаса. Стоило ли исправлять или дожидаться недовольного пользователя? Как считаете?

Всего голосов 5: ↑5 и ↓0+5
Это сколько же у них тогда ошибок в Офисе?
Всего голосов 7: ↑4 и ↓3+1
НЛО прилетело и опубликовало эту надпись здесь

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



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

Всего голосов 24: ↑18 и ↓6+12

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

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

Однако, в «военное время» компилятор (или даже линкер) может объединять константы (в том числе константные строки), и вот тогда сравнение будет работать правильно. Причем подобное не редкость в embedded-проектах.
Всего голосов 7: ↑7 и ↓0+7
И такая же фигня и в 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");
        }
    }


Что вот выведет?
Комментарий пока не оценивали0
НЛО прилетело и опубликовало эту надпись здесь

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


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

Всего голосов 9: ↑9 и ↓0+9
Я когда писал приложение на Андроид случайно сравнил одинаковые строки через ==
Они оказались неравны.
Хотя это были переменные разных объектов.
Так что дело либо в этом, либо в Google JVM
Комментарий пока не оценивали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.
Всего голосов 2: ↑1 и ↓10
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь

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

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

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

Всего голосов 1: ↑1 и ↓0+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";
Всего голосов 1: ↑1 и ↓0+1
Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу. Раз в сообщении выше показано, что точка останова сработала, значит как раз ссылка на константу и была передана в функцию
А посмотреть исходник? Благо даже скачивать не надо, github позволяет браузить репозиторий online. В оригинале был массив
wchar_t m_resolvedName[100];
Всего голосов 2: ↑2 и ↓0+2
Из контекста в коде калькулятора непонятно, массив там передаётся или ссылка на текстовую константу.

Ох… Ну сколько можно… В статье чётко написано:
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. Уже несколько раз это обсудили в комментариях.

Всего голосов 8: ↑8 и ↓0+8
НЛО прилетело и опубликовало эту надпись здесь
Вы осознаёте, что
const wchar_t *A = L"test";
wchar_t B[] = L"test";
принципиально разные сущности в C++?
Всего голосов 2: ↑2 и ↓0+2
НЛО прилетело и опубликовало эту надпись здесь
В статьях мы тщательно подходим к выписыванию фрагментов кода, делая их по возможности самодостаточными для демонстрации ошибки. С самого начала в статье приводился следующий фрагмент кода:
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. Только не говорите, что и ссылки на документацию тоже не было :).
Всего голосов 1: ↑1 и ↓0+1
НЛО прилетело и опубликовало эту надпись здесь
2. С массивом тоже будет «undefined behaviour», а не «always false».
Объясните, почему?
Массив — член класса его адрес считается как смещение поля от адреса объекта. Строковый литерал — статическое значение. Не важно, размещено в пуле или ещё где-то в секции read-only data. Почему адреса могут совпасть? Разве в стандарте запрещено сравнение указателя с литералом? Вероятно, тут CLion не прав.
Всего голосов 3: ↑3 и ↓0+3
НЛО прилетело и опубликовало эту надпись здесь
Ленивые присваивания строк.
Это касается только классов-обёрток (типа std::string), но никак не Си-шных «строк» (char*)
Всего голосов 2: ↑2 и ↓0+2
НЛО прилетело и опубликовало эту надпись здесь
Скорее всего не понадобится ни везение, ни фазы луны. VS использует string pooling при настройках по-умолчанию что в debug, что в release сборке.
Всего голосов 3: ↑1 и ↓2-1
Тут речь о том, что сообщение PVS-Studio некорректное или неполное, что привело к неправильной интерпретации этого сообщения даже у вас в статье. Сравнение не будет всегда false для сравнения указателей(!), это показывает практика string pooling в VS. В данном конкретном случае проблема не в сравнении указателей, а в том, что m_resolvedName это массив и в С++ нужно очень сильно извратиться, чтобы условие сработало, если это вообще возможно. В данном случае PVS-Studio должна была написать именно это — переменная массив и условие потенциально никогда не выполнится.

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

Как я понял, товарищ Delics именно это и хотел донести, но его почему-то минусуют.
Комментарий пока не оценивали0
В данном случае 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 с длиной, это не замена оператору == ).
Всего голосов 1: ↑1 и ↓0+1
Может, для кого-то и надо так разжёвывать, но как по мне «expression is always false» тут самое лучшее

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

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

Комментарий пока не оценивали0
Ставлю сотку приложение спрототипировали на C# и потом переписали на С++ CLI/CX
Но зачем?
Всего голосов 4: ↑2 и ↓20
Хороший ответ на «эти ошибки никому не мешают». Человек не поленился в выходной поправить ошибку в рабочем проекте.
Всего голосов 7: ↑7 и ↓0+7
Ну стыдно же:) Если в вашем проекте такое найдут и в новостях напишут, я думаю вы тоже подорветесь править.
Всего голосов 2: ↑2 и ↓0+2
Мне только интересно, эти исправления сделаны на публику в гитхабе, или они попали во внутренние репозитории MS, из которых собирается продукт. Узнаем со следующим апдейтом, будет ли туда включен calc.exe.
Всего голосов 6: ↑5 и ↓1+4
в выходной

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

Всего голосов 3: ↑3 и ↓0+3

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

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

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

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

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

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

Другое дело, если бы кто-то нашёл архитектурные ошибки и глобально отрефакторил — тогда бы код улучшился и его дешевле было бы развивать в будущем. Но что-то таких героев не видно.
Всего голосов 6: ↑3 и ↓30
Главная мысль это то что для коммерческого успеха идеальный код не нужен. Это на заметку собеседющим, главное это человеческие качества, имхо основное это умение довести до конца.
Всего голосов 9: ↑7 и ↓2+5
Ну и приложение из стандартной поставки Windows это тоже не «коммерческий успех», прошу заметить — кто его самостоятельно ставить-то или покупать будет? Так что мысль хорошая, только далековата от предмета статьи =)
Всего голосов 2: ↑2 и ↓0+2
Стандартная поставка влияет на коммерческий успех Windows. Другое дело что калькулятор вряд ли вносит хоть сколько-то заметный вклад в этот успех, поэтому практически вы правы.
Комментарий пока не оценивали0
А что это за интересный синтаксис:
String^
Всего голосов 1: ↑1 и ↓0+1
Это C++/CLI.
Всего голосов 3: ↑3 и ↓0+3
Спасибо. Оказывается добавляет 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.)
Всего голосов 2: ↑2 и ↓0+2
Докатились. Калькулятор не можем написать без кучи багов и неоднозначного кода.
А сразу правильно что не пишется? Благо логики в калькулаторе минимум.
Всего голосов 19: ↑1 и ↓18-17
Похоже, калькулятор это первые шаги перед переводом чего-то большого в опенсорс. Так же было вначале с .net, сначала библиотеки, unit тесты и тд.
Комментарий пока не оценивали0

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

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

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

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

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

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

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

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

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

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

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

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

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


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


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

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

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

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

Да, калькулятор сильно людям понравился. Вроде обычная статья от 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 исправляло эти баги.

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

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

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

Да и вопрос — зачем? Показать что в таком огромном проекте туева хуча опечаток допущенных по глупости? Реклама статическому анализатору для разработчиков из Microsoft? Ну, может быть, только как-то сомнительно выглядит. Лучше все же как сейчас, на максимально широкую аудиторию работать.
Всего голосов 3: ↑3 и ↓0+3
Для изучения есть абсолютно легальный WRK.
Комментарий пока не оценивали0
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь
НЛО прилетело и опубликовало эту надпись здесь
Зарегистрируйтесь на Хабре, чтобы оставить комментарий

Ваш аккаунт

Разделы

Информация

Услуги

Техническая поддержка
© 2006–2024, Habr