How to become an author
Search
Write a publication
Pull to refresh

Comments 25

Самим лучше не исправлять, а уведомить об этом разработчиков. Но ребята уже сделали это.
Total votes 10: ↑10 and ↓0+10
Действительно, исправлять ошибки должны ТОЛЬКО разработчики проекта. Увы, уже были случаи, когда сторонний человек вдохновившись нашей статьей, успевал «наломать дров».
Total votes 5: ↑5 and ↓0+5
UFO just landed and posted this here
Не все команды готовы вкладываться в качество кода. Это определенный уровень развития процессов разработки. Команда (ну к примеру ее тим-лид, если говорить конкретно) должна задать вопрос: «А что ЕЩЕ мы можем сделать для того, чтобы наш код был более качественный?» И тогда ответом будет: «Например, использовать статический анализатор кода».
Total votes 1: ↑1 and ↓0+1
UFO just landed and posted this here
На самом деле у них очень качественный код. Просто в серьезном проекте всегда можно что-то найти.
Total votes 9: ↑9 and ↓0+9
И от размера зависит. Чем больше размер проекта, тем больше там ошибок. Люди ошибаются, увы.
Total votes 2: ↑2 and ↓0+2
Возможно статический анализатор у них есть, но правила настроены по другому.
И не всегда статический анализ показывает на ошибки. Иногда просто на сложный(но возможно быстрый) код.
This comment wasn’t rated yet0
Если сравнивать результаты анализатора, то как на ваш взгляд — на фоне других открытых проектов поделие от Микрософт выглядит лучше или хуже?
Total votes 5: ↑1 and ↓4-3
Если сравнить результаты, то по собственному опыту могу сказать, что если в проекте не использовались инструменты статического анализа, то там с большой вероятностью есть ошибки. В любом проекте.

Мы проверяли проекты, которые были всегда отрытыми, и писали статьи. Проверяли только что открытые проекты Microsoft и других компаний, и также пишем статьи.
Total votes 1: ↑1 and ↓0+1
Объясните, зачем анализатор выдает предупреждения на сравнение беззнаковых чисел в assert-ах? assert стоит для того, чтобы гарантировать, что условие не нарушится, если вдруг переменная, участвующая в assert-е поменяет тип на знаковый. Смысл выдавать здесь предупреждение?
This comment wasn’t rated yet0
Вообще во многих случаях PVS-Studio молчит, когда встречает assert-ы. Есть десятки исключений на эту тему. Но иногда есть смысл придираться и к ним.

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

Но в 90% случаев это просто ошибка и человек написал assert, который не выполняет своего предназначения.

Рассмотрим случай, приводимый в статье:

AssertMsg(srcIndex - src->left >= 0,
    "src->left > srcIndex resulting in \
     negative indexing of src->elements");

Человек хотел защитится assert-ом от неправильного индекса. Лучше поймать assert(), чем разбираться почему программа упадёт при вызове последующей функции js_memcpy_s.

Но на самом деле assert() ничего не проверяет. Условие всегда истинно. Ассерт не сработает, функция сгенерирует исключения. Таким образом программист не добился своей цели, когда писал assert().
Total votes 3: ↑3 and ↓0+3
Да, тут я упустил из виду, что проверяется не то условие, которое подразумевается. Но в других ваших статьях часто указывается как ошибка проверка какой-либо беззнаковой переменной (а не выражения, как здесь) в assert-е на >= 0. Вот в этих случаях мне кажется, что предупреждение выдавать не нужно. Так можно и юнит тесты (да и вообще все тесты) выкидывать — а зачем они, если анализ показывает, что ошибок нет… пока нет и пока анализатор запускается.
Total votes 1: ↑1 and ↓0+1
Да, бывают ложные срабатывания. Но часто имелось совсем не то в виду, что получилось.

Например, функция возвращает -1 в случае ошибки. Предполагается, что такой ситуации в программе не будет, но решают написать assert() на всякий случай. В результате имеем:

unsigned i = foo();
assert(i >= 0);


Вновь assert() не выполняет назначенной для себя функции.

И посмотрите, сколько блестящих фрагментов находит диагностика V547! Так что лучше ругаться :). Это ведь шедевры:

inline void CXmlReader::CXmlInputStream::UnsafePutCharsBack(
  const TCHAR* pChars, size_t nNumChars)
{
  if (nNumChars > 0)
  {
    for (size_t nCharPos = nNumChars - 1;
         nCharPos >= 0;
         --nCharPos)
      UnsafePutCharBack(pChars[nCharPos]);
  }
}


This comment wasn’t rated yet0
Например, функция возвращает -1 в случае ошибки.

В таком случае достаточно ругани на то, что знаковая переменная записывается в беззнаковую (а надеюсь, такое есть?), именно тут создается ошибка.
И посмотрите, сколько блестящих фрагментов находит диагностика V547! Так что лучше ругаться :). Это ведь шедевры:

Так я не спорю, что в обычных фрагментах кода нужно выдавать это предупреждение. Я говорю, что в assert-ах оно неуместно, когда у нас сценарий assert(unsignedVar >= 0). В таком сценарии я вижу только подстраховку на тот случай, если тип станет знаковым, ну и явно подчеркнуть намерение в коде, что отрицательных чисел нет (так как глядя на текст, тип переменной может быть не видно).
This comment wasn’t rated yet0
В таком случае достаточно ругани на то, что знаковая переменная записывается в беззнаковую (а надеюсь, такое есть?)

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

char *a = "foo";

Этот код вообще компилироваться не должен, так как «foo» это константная строка, а мы помещаем её в неконтактную. Но столько кода уже понаписано, что поздно пить боржоми.

This comment wasn’t rated yet0
char *a = "foo";

Это все же немного другой пример. Тут информация не теряется и не меняется, в отличие от молчаливого приведения типа из знакового в беззнаковый (и наоборот, а также усекания более крупных типов). Фактически, единственное, что создает такой код — перекладывает часть проверок на из compile-time на runtime (вместо ругани компилятора получим Aссess violation при попытке записать в такой указатель).

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

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

Но ведь это такие грабли могут быть скрыты! Как это безсмысленно ругаться? На неявные стоит ругаться (а современные компиляторы так и делают, к счастью). А особенно стоит, если есть подозрение, что функция возвращает код ошибки. Кстати, предложение для новой диагностики: если функция возвращает знаковый тип, который сохраняется в беззнаковую переменную, которая в свою очередь потом проверяется на >= 0 — полагать, что она возвращает отрицательные коды ошибок и ругаться именно на место сохранения результата (в дополнение к ругани на безсмысленность сравнения >= 0, за исключением случаев assert-ов, как я писал выше).
This comment wasn’t rated yet0
V595 The 'instrLd' pointer was utilized before it was verified against nullptr. Check lines: 1823, 1831. flowgraph.cpp 1823
IR::Instr *
FlowGraph::PeepTypedCm(IR::Instr *instr)
{
…
if (instrLd && !instrLd->GetSrc1()->IsEqual(instr->GetDst()))
{
return nullptr;
}

if(instrLd2 && !instrLd2->GetSrc1()->IsEqual(instrLd->GetDst()))
{
return nullptr;
}
…
}

А разве тут есть ошибка? Ведь, если instrLd == null, то сработает еще первое условие и произойдет return nullptr, до второго даже не дойдет. Или речь про instr?
This comment wasn’t rated yet0
Не сработает. Если instrLd == nullptr то в случае когда instrLd2 != nullptr вызовется instrLd->GetDst() — вот тут и проблема. Все-таки сложно без анализатора находить такие места, ага.
Total votes 3: ↑3 and ↓0+3
Они ещё не видели, что в CNTK нашлось..., но всему своё время ;-)
This comment wasn’t rated yet0
Sign up to leave a comment.

Your account

Sections

Information

Services

Support
© 2006–2024, Habr