Pull to refresh

Comments 46

Иллюстрация к этому посту просто мега-крутая.
Хоть я и не пишу на C/C++, но всегда с удовольствием читаю ваши статьи.
Некоторые ошибки оказываются действительно очень замысловатыми.
Да и вообще, подобная модель продвижения продукта мне очень нравится. Побольше бы компаний показывали, как можно использовать их софт в реальных условиях, то есть выводить знакомство за рамки Tutorial/FAQ.
Проприентарное ПО. И нет, это не комплимент.
Конечно не комплимент :D, Пишется-то проприЕТарное
Кстати, есть проект freeswitch, его не задумывались?
А это, однако же, интересное занятие — с помощью мощного инструмента искать несовершенства и даже откровенные ошибки в чужом коде!

P.S. Не устану повторять — QT != Qt
Согласитесь, что cPPcAT или pVs-sTuDiO выглядит не очень? А на кьюте свет клином сошелся и половина игнорирует верное название!
Происходит это (QT/Qt) не со зла. Очень сложно уследить, чтобы люди правильно использовали название.

P.S. Когда-нибудь я напишу статью со всеми вариантами как люди называют PVS-Studio. Десяток вариантов наберется сразу, еще десяток — если повспоминать и покопаться в почте.
Они (вы) неконтактные какие-то. Раз им (вам) это не нужно, то нам зачем?
Я бы так не сказал, ведь почти все найденные баги по результатам прошлых тестов были оперативно устранены.
Наш проект развивается, и Ваш инструмент развивается. Интересно какими окажутся результаты теста\проверки в этот раз.
Мы пробовали общаться с Брагиным над предмет какого-либо сотрудничества. Общения не вышло.
Рискну лишь предположить (всей картины я не знаю), что в Вашем предложении была серьезная коммерческая составляющая.

Просто для сравнения, без всякого скрытого упрека скажу, что например Coverty переодически проводят статистический анализ кода ReactOS бесплатно и абсолютно безусловно.
Просто для сравнения, у них есть грант на проверку open-source, а у нас нет.
В той или иной степени использую 9 из перечисленных Open-source проектов. Если качество их кода повышается, я (буду) только благодарен PVS-Studio. В некоторых статьях мелькала информация о предоставлении ключа разработчикам. Вовсе не пользуюсь проприетарным ПО, но за такие шаги готов вас советовать знакомым разработчикам.
Советуйте смело, не стесняйтесь! Мы действительно сотрудничаем (в плане ключей и поддержки) с некоторыми разработчиками. Далеко не всеми, конечно, но тем не менее.
Нужно проверить код PVS-Studio с помощью PVS-Studio. -)
Так, вопрос про проверку самих себя есть, про Linux есть. Ещё один извечный вопрос остался.
Ответ: Да.
Ждём версии под Linux, работающей без встраивания в IDE.
И все-таки, много инспекций реализованы не совсем так, как это сделал бы я. Например, вы ругаетесь на sizeof(x)/sizeof(x[0]), но если x имеет тип LPTSTR или char*, не лучше ли предложить _tcslen()/strlen()? И правильно ли ругаться на это всегда? Ведь для нестроковых массивов этот метод вроде бы приемлем, или нет?
Видимо речь идёт о V511 (примеры) или V514 (примеры).

Да, для случая работы с «char *» можно рекомендовать использовать strlen(). Однако, это уже второстепенно. Главное, что найдена ошибка. Если она найдена, проблем с правками не будет. Рекомендация использовать strlen() может, как помочь, так и только запутать. Вдруг это массив абстрактных байт.

Представьте, что ошибка в заполнении массива вида:
typedef unsigned char * BYTE;
void SetBlack(BYTE rgba[4])
{
  memset(rgba, 0, sizeof(rgba) / sizeof(rgba[0]);
}

И тут вдруг мы говорим, что надо использовать strlen(). Люди невнимательны и могут «забанить» такое предупреждение, посчитав, что анализатор сглупил. Ну причём здесь strlnen() какой-то… Мне часто приходится объяснять в почте по поводу разного кода, что это не ложное срабатывание, а действительно ошибка. Люди очень невнимательны при чтении предупреждений и своего кода.

Так что неверное анализатору не стоит излишне умничать и пытаться додумать код за программиста.

> Ведь для нестроковых массивов этот метод вроде бы приемлем, или нет?

Хочу остановится на этом моменте. Да, конечно деление часто приемлемо и предупреждение выдается только для анормальных ситуаций.

У меня из головы не выходит один свежих из комментариев к одной из наших статей. Нам написали, что наша беда — «проблема айсберга». Дело в том, что огромная часть нашей работы скрыта от глаз и пользователь видит только сообщения анализатора. Часть которых в добавок ложны. Не имея возможности оценить объем работы, он думает " я такой инструмент за пару недель сделаю, если поработаю как следует". В результате, очень сложно продать инструмент, который «можно сделать за 2 недели». Тут и $250 это много. И поэтому, Вам тяжело продавать.

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

Печально, когда кто-то думает, что мы ругаемся на все конструкции sizeof(x)/sizeof(x[0]). Тут и за $25 не продать.

Если бы мы так подходили, то проще на каждую непустую строку программы ругаться. Ведь вполне там может быть ошибка. :)

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

Тот случай, когда комментарий тянет на отдельную статью!
Так стоп, я не понял, вот вы пишете что
Печально, когда кто-то думает, что мы ругаемся на все конструкции sizeof(x)/sizeof(x[0])
Тогда я не понимаю, что конкретно стало критерием для warning’а в тех местах, которые приведены у вас на сайте. Почему именно они, а не другие? Если есть какой-то контекст, мне кажется нужно его показывать. Если это warnings показываются только на типах которые похожи на строки, тогда нелогично утверждать что
Рекомендация использовать strlen() может, как помочь, так и только запутать
т.к. фактически, вы использовали критерий «строковости» для того чтобы ошибку выловить и показать пользователю, но считаете зазорным упомянуть об этом. Если бы речь шла не о системе анализа а о системе анализ+коррекция, я бы ожидал в этом месте контекстного действия в стиле replace with strlen/_tcslen().

Более того, додумывание сценариев для программиста – это именно то, чем PVS занимается в первую очередь т.к. очень много ваших инспекций являются по определению opinionated. Например если я напишу if (x[0] != y[0]) { ... } if (x[1] != y[0]) вы вероятно будете рекоммендовать мне проверить мой код на предметнеправильных индексов (V525) несмотря на то что этот пример — как раз то место где, возможно, не стоило бы додумывать за программиста информацию. Мало ли как я адресую массивы. У меня например многие модели сгенерированы из Excel, там такие инспекции скорее вредят, но я не жалуюсь – false positive всяко лучше чем пропущенная ошибка.

Вообщем, резюмируя, мне не очень ясна логика по которой эта инспекция применяется. Расскажите?
<зануда=on>Можно почитать документацию. Она всё объясняет и содержит поясняющие примеры: V511, V514.<зануда=off>

т.к. фактически, вы использовали критерий «строковости» для того чтобы ошибку выловить и показать пользователю, но считаете зазорным упомянуть об этом.

Строки тут вообще ни при чем. Это частный случай. Да, со строками чаще всего проблемы, но всё равно это частный случай.

V511
(пример из документации)
void Foo(float array[3])
{
  size_t n = sizeof(array) / sizeof(array[0]);
  for (size_t i = 0; i != n; i++)
    array[i] = 1.0f;
}


sizeof(array) — вычисляет размер УКАЗАТЕЛЯ, а не массива. Фича Си/Си++. В функцию передаётся не массив из трёх элементов, а просто указатель. То, что в скобках написано [3] вообще ничего не значит. Просто подсказка программисту. Это указатель.

Предназначена диагностика для ловли таких ситуаций:
ID_INLINE mat3_t::mat3_t( float src[ 3 ][ 3 ] ) {
  memcpy( mat, src, sizeof( src ) );
}


Скопируем только 4 байта в Win32 (или 8 в Win64).

Тут деление и строки вообще ни при чем.

P.S. Но если деление будет, диагностика V511 проявит себя вместе с V514. Вообще, нередко одна ошибка проявляет себя более чем одним предупреждением.

V514
(пример из примеров ошибок)

struct ObjectGroups{
  componentList objectGrpCompList;
  int objectGroupId;
  short objectGrpColor;
  void write(FILE* file) const;
}* objectGroups;

void write(FILE* file) const
{
  size_t size = sizeof(objectGroups)/sizeof(ObjectGroups);
  for(size_t i=0; i<size; ++i)
  {
    objectGroups[i].write(file);
    if(i+1<size) fprintf(file," ");
  }
}


Делим размер УКАЗАТЕЛЯ на _неважно что_. Бессмыслица. Здесь вообще 1 получится, так как делим размер указателя на размер указателя

Строки опять только частный случай.

По поводу доделывания. Да додумывает. Но зачем приплетать strlen(), если диагностики строились вообще из других соображений. V511 — люди не знают про фичу языка. V514 — случайно делят размер указателя на что-то.

Очень хорошая дискуссия получилась, которая показывает, что всё намного сложней, чем кажется со стороны. Спасибо. Будем думать, как показать всю глубину наших глубин.
А, все, понял. Почему-то показалось что вы против этого целиком. Вообще лучше когда даются хинты «как чинить». Вот если бы там было написано ‘Calculation based on array decayed to pointer; consider replacing with float (&arr)[3]’, вопросов бы не было.
Проверяли. Ничего интересного. Буквально пара плохих мест в духе:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 7477, 7523. Format tcformat.c 7477

BOOL CALLBACK PageDialogProc(....)
{
  ...
  else if (nCurPageNo == HIDDEN_VOL_HOST_PRE_CIPHER_PAGE)
  ...
  else if (nCurPageNo == HIDDEN_VOL_HOST_PRE_CIPHER_PAGE)
  ...
}


V547 Expression 'fileDataEndPos < 0' is always false. Unsigned type value is never < 0. Setup selfextract.c 551

BOOL SelfExtractInMemory (char *path)
{
  unsigned int fileDataEndPos = 0;
  ...
  if (fileDataEndPos < 0)
  {
    Error ("CANNOT_READ_FROM_PACKAGE");
    return FALSE;
  }
  ...
}
UFO just landed and posted this here
Проверьте библиотеку Qt. При их огромных объемах кода наверняка найдете кучу ошибок.
Не очень понятно, что такое «проверьте Qt». Нет же одного солюшена, который бы назывался Qt.

Это если игнорировать указанную в списке статью про проверку Qt :-).
Под Qt понимается библиотека Qt. К тому же вы проверяли 4 версию-которая уже не очень популярна. Лучше 5 версию проверьте
Проверьте пожалуйста и Threading Building Blocks (https://www.threadingbuildingblocks.org/download#stable-releases). Полагаю в Intel проверяют код своим статическим анализатором кода. Будет интересно узнать результаты.
Некоторые Интеловские проекты есть в списке выше. Ошибок хватает :-).
Sign up to leave a comment.