15.08.2016 14:40
EgorBredikhin 48 13400 Источник
В этой статье речь пойдет о проверке еще одного известного open source проекта — векторного графического редактора Inkscape 0.92. Проект развивается уже более 12 лет и предоставляет множество возможностей по работе с различными форматами векторных иллюстраций. За это время его кодовая база выросла до 600 тысяч строк, и пришло время проверить его с помощью статического анализатора PVS-Studio.


Введение


Inkscape — это кроссплатформенный свободный векторный графический редактор. Он широко используется любителями и профессионалами по всему миру для создания иллюстраций, иконок, логотипов, диаграмм, карт, а также веб-графики. Inkscape стал одним из самых популярных редакторов в своей области. Проект был создан в 2003 году как форк проекта Sodipodi, и до сих пор продолжает развиваться. Подробнее про Inkscape можно прочитать на официальном сайте.

Для проверки использовалась последняя версия Inkscape — 0.92, код которой доступен в репозитории на GitHub и статический анализатор PVS-Studio 6.07, загрузить который можно по ссылке. Правда, на момент написания статьи для скачивания доступна только PVS-Studio для Windows. Но ситуация скоро изменится. И можно уже заранее записаться в добровольцы для тестирования бета-версии PVS-Studio для Linux. Подробности можно узнать из статьи: "PVS-Studio признаётся в любви к Linux".



Но вернемся к ошибкам. Хочу отметить, что в статье были выбраны и описаны наиболее интересные сообщения анализатора. Для более тщательной проверки авторы проекта смогут получить у нас временный ключ для PVS-Studio и отчёт. Так как публичной PVS-Studio пока нет, для просмотра отчёта они смогут воспользоваться инструментом PVS-Studio Standalone, работающим под Windows. Да, это не удобно. Но прошу всех потерпеть, осталось не так долго до счастливого момента выхода PVS-Studio для Linux.

Результаты проверки


Проверка указателя на null после new


Предупреждение PVS-Studio: V668 There is no sense in testing the 'outputBuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. gzipstream.cpp 180
bool GzipInputStream::load()
{
    ....
    outputBuf = new unsigned char [OUT_SIZE];
    if ( !outputBuf ) {  // <=
        delete[] srcBuf;
        srcBuf = NULL;
        return false;
    }
    ....
}

Согласно современному стандарту C++, при невозможности выделить память оператор new генерирует исключение std::bad_alloc(), а не возвращает nullptr. В случае, если системе не удастся выделить память, будет выброшено исключение и выполнение функции прекратится, следовательно, программа никогда не зайдет в блок после условия.

В данном случае это может привести к утечке памяти. Самым очевидным решением проблемы является использование блока try {....} catch(const std::bad_alloc &) {....}, но гораздо лучше вместо явного освобождения памяти использовать умные указатели (smart pointers).

Аналогичные проверки указателей:

  • V668 There is no sense in testing the 'destbuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. gzipstream.cpp 397
  • V668 There is no sense in testing the 'srcBuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. gzipstream.cpp 175
  • V668 There is no sense in testing the 'oldcurve' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. sp-lpe-item.cpp 719


Сравнение this с нулем


Предупреждение PVS-Studio: V704 '!this' expression in conditional statements should be avoided — this expression is always false on newer compilers, because 'this' pointer can never be NULL. sp-lpe-item.cpp 213

bool SPLPEItem::performPathEffect(....) {
    if (!this) {
        return false;
    }
    ....
}

Согласно современному стандарту С++, указатель this никогда не может быть нулевым. Зачастую использование сравнения this с нулем может приводить к неожиданным ошибкам. Подробнее прочитать об этом можно в описании диагностики V704.

Ещё одна проверка на равенство this значению nullptr:

  • V704 'this' expression in conditional statements should be avoided — this expression is always true on newer compilers, because 'this' pointer can never be NULL. sp-paint-server.cpp 42


Опасное переопределение параметра


Предупреждение PVS-Studio: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 1046, 1051. sp-mesh-array.cpp 1051

void SPMeshNodeArray::create( ...., Geom::OptRect bbox ) // <=
{
  ....
  if( !bbox ) {
    std::cout << "SPMeshNodeArray::create(): bbox empty" 
              << std::endl;
    Geom::OptRect bbox = item->geometricBounds();        // <=
  }
  if( !bbox ) {                                          // <=
    std::cout << "ERROR: No bounding box!" 
              << std::endl;
    return;
  }
  ....
}

По задумке автора, в случае, когда параметр bbox равен nullptr, для него должен создаться новый объект типа Geom::OptRect, и если объект создать не удалось, то происходит выход из метода с сообщением об ошибке.

Однако, код работает совсем не так, как ожидал автор. Когда параметр bbox равен nullptr, внутри первого блока if происходит создание совершенно нового объекта bbox, который сразу же уничтожается при выходе из этого блока. В результате получается, что второе условие выполняется всегда, когда выполняется и первое, поэтому каждый раз, когда параметр bbox равен nullptr, происходит выход из метода с сообщением об ошибке.

Данный код следовало бы написать так:

void SPMeshNodeArray::create( ...., Geom::OptRect bbox )
{
  ....
  if( !bbox ) {
    std::cout << "SPMeshNodeArray::create(): bbox empty" 
              << std::endl;
    bbox = item->geometricBounds();
    if( !bbox ) {
      std::cout << "ERROR: No bounding box!" 
                << std::endl;
      return;
    }
  }
  ....
}


Неправильно закомментированная строка


Предупреждение PVS-Studio: V628 It's possible that the line was commented out improperly, thus altering the program's operation logics. FontFactory.cpp 705

font_instance *font_factory::Face(....)
{
  ....
  if( features[0] != 0 ) // <=
    // std::cout << "          features: " << std::endl;

  for( unsigned k = 0; features[k] != 0; ++k ) {
  // dump_tag( &features[k], "            feature: ");
  ++(res->openTypeTables[ extract_tag(&features[k])]);
  }
  ....
}

Здесь автор забыл закомментировать строку с условием, которая использовалась для отладки. В данном случае повезло и это не привело к негативным последствиям. Получилось, что условие в if просто продублировало условие выполнения цикла for на первой итерации, но это несомненно является ошибкой и может привести к проблемам в будущем.

«Одноразовый цикл»


Предупреждение PVS-Studio: V612 An unconditional 'break' within a loop. text_reassemble.c 417

int TR_kern_gap(....)
{ 
  ....
  while(ptsp && tsp){
    ....
    if(!text32){
      ....
      if(!text32)break;
    }
    ....
    if(!ptxt32){
      ....
      if(!ptxt32)break;
    }
    ....
    break; // <=
  }
  ....
  return(kern);
}

Этот цикл в любом случае завершится после первого прохода, поскольку перед оператором break нет никакого условия. Точно сказать что подразумевал автор сложно. Если ошибки здесь нет, то код всё равно лучше переписать и заменить while на if.

Очень странный метод


Предупреждение PVS-Studio: V571 Recurring check. The 'back == false' condition was already verified in line 388. Path.cpp 389

void
Path::SetBackData (bool nVal)
{
  if (back == false) {
    if (nVal == true && back == false) {
      back = true;
      ResetPoints();
    } else if (nVal == false && back == true) {
      back = false;
      ResetPoints();
    }
  } else {
    if (nVal == true && back == false) {
      back = true;
      ResetPoints();
    } else if (nVal == false && back == true) {
      back = false;
      ResetPoints();
    }
  }
}

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

void
Path::SetBackData (bool nVal)
{
  back = nVal;
  ResetPoints();
}


Потерянная запятая


Предупреждение PVS-Studio: V737 It is possible that ',' comma is missing at the end of the string. drawing-text.cpp 272
void DrawingText::decorateStyle(....)
{
  ....
  int dashes[16]={
     8,  7,   6,   5,
     4,  3,   2,   1,
    -8, -7,  -6,  -5  // <=
    -4, -3,  -2,  -1
  };
  ....
}

Была пропущена запятая, что приводит к тому, что массив dashes будет проинициализирован совсем не такими значениями, которые ожидал автор.

Ожидалось:

{ 8,  7,  6,  5,
  4,  3,  2,  1,
 -8, -7, -6, -5,
 -4, -3, -2, -1 }

На самом деле массив будет заполнен так: 

{ 8,  7,  6,  5, 
  4,  3,  2,  1,
 -8, -7, -6, -9,
 -3, -2, -1,  0 }

На место 12-го элемента массива будет записано число -5 — 4 == -9. А последний элемент (на который не хватило элементов в списке инициализации массива) будет, согласно стандарту C++, инициализирован нулём.

Неверная длина в strncmp


Предупреждение PVS-Studio: V666 Consider inspecting third argument of the function 'strncmp'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. blend.cpp 85

static Inkscape::Filters::FilterBlendMode
 sp_feBlend_readmode(....) {
  ....
  switch (value[0]) {
    case 'n':
      if (strncmp(value, "normal", 6) == 0)
        return Inkscape::Filters::BLEND_NORMAL;
      break;
    case 'm':
      ....
    case 's':
      if (strncmp(value, "screen", 6) == 0)
          return Inkscape::Filters::BLEND_SCREEN;
      if (strncmp(value, "saturation", 6) == 0) // <=
          return Inkscape::Filters::BLEND_SATURATION;
      break;
    case 'd':
      ....
    case 'o':
      if (strncmp(value, "overlay", 7) == 0)
          return Inkscape::Filters::BLEND_OVERLAY;
      break;
    case 'c':
      ....
    case 'h':
      if (strncmp(value, "hard-light", 7) == 0) // <=
          return Inkscape::Filters::BLEND_HARDLIGHT;
      ....
      break;
    ....
  }
}

В функцию strncmp передается неверная длина строк «saturation» и «hard-light», поэтому будут сравниваться не все символы, а только первые 6 и 7 символов соответственно. Скорее всего, здесь проявило себя т.н. Copy-Paste программирование. Эта ошибка приведет к ложным срабатываниям при добавлении новых элементов в switch-case. Стоило бы исправить код:
if (strncmp(value, "saturation", 10) == 0)
....
if (strncmp(value, "hard-light", 10) == 0)


Потенциальное деление на ноль


Предупреждение PVS-Studio: V609 Divide by zero. Denominator range [0..999]. lpe-fillet-chamfer.cpp 607

Geom::PathVector
LPEFilletChamfer::doEffect_path(....)
{
  ....
  if(....){
    ....
  } else if (type >= 3000 && type < 4000) {
      unsigned int chamferSubs = type-3000;
      ....
      double chamfer_stepsTime = 1.0/chamferSubs;
      ....
  }
  ...
}

В случае, когда переменная type будет равна 3000, значение переменной chamferSubs составит 0. Соответственно, значение chamfer_stepsTime будет равно 1.0/0 == inf, а это явно не то, чего ожидает автор. Чтобы избежать подобной ситуации стоит изменить условие в блоке if:

...
else if (type > 3000 && type < 4000)
...

Или же можно отдельно обрабатывать ситуацию, когда chamferSubs == 0.

Аналогичная ситуация:

  • V609 Divide by zero. Denominator range [0..999]. lpe-fillet-chamfer.cpp 623


Пропущенный else?


Предупреждение PVS-Studio: V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. sp-item.cpp 204

void SPItem::resetEvaluated() 
{
  if ( StatusCalculated == _evaluated_status ) {
    ....
  } if ( StatusSet == _evaluated_status ) { // <=
      ....
  }
}

Судя по форматированию кода (оператор if расположен на той же строке, что и закрывающаяся скобка от предыдущего if) и логике работы, здесь было пропущено ключевое слово else:

....
if ( StatusCalculated == _evaluated_status ) {
    ....
  } else if ( StatusSet == _evaluated_status ) {
      ....
  }
}
....


Работа с нулевым указателем


Предупреждение PVS-Studio: V595 The 'priv' pointer was utilized before it was verified against nullptr. Check lines: 154, 160. document.cpp 154

SPDocument::~SPDocument() 
{
  priv->destroySignal.emit();                      // <=
  ....
  if (oldSignalsConnected) {
    priv->selChangeConnection.disconnect();        // <=
    priv->desktopActivatedConnection.disconnect(); // <=
  } else {
    ....
  }
  if (priv) {                                      // <=
    ....
  }
  ....
}

В нижнем блоке if происходит проверка priv на NULL, т.к. автор допускает равенство этого указателя нулю, однако, выше указатель уже используется и без всяких проверок. Чтобы исправить эту ошибку следует проверить значение указателя до того, как его использовать.

Аналогичные предупреждения:

  • V595 The 'parts' pointer was utilized before it was verified against nullptr. Check lines: 624, 641. sp-offset.cpp 624
  • V595 The '_effects_list' pointer was utilized before it was verified against nullptr. Check lines: 103, 113. effect.cpp 103
  • V595 The 'num' pointer was utilized before it was verified against nullptr. Check lines: 1312, 1315. cr-tknzr.c 1312
  • V595 The 'selector' pointer was utilized before it was verified against nullptr. Check lines: 3463, 3481. cr-parser.c 3463
  • V595 The 'a_this' pointer was utilized before it was verified against nullptr. Check lines: 1552, 1562. cr-sel-eng.c 1552
  • V595 The 'FillData' pointer was utilized before it was verified against nullptr. Check lines: 5898, 5901. upmf.c 5898
  • V595 The 'event_context' pointer was utilized before it was verified against nullptr. Check lines: 1014, 1023. tool-base.cpp 1014
  • V595 The 'event_context' pointer was utilized before it was verified against nullptr. Check lines: 959, 970. tool-base.cpp 959
  • V595 The 'this->repr' pointer was utilized before it was verified against nullptr. Check lines: 662, 665. eraser-tool.cpp 662
  • V595 The 'this->repr' pointer was utilized before it was verified against nullptr. Check lines: 662, 665. eraser-tool.cpp 662
  • V595 The 'modified_connection' pointer was utilized before it was verified against nullptr. Check lines: 1114, 1122. gradient-vector.cpp 1114
  • V595 The 'c' pointer was utilized before it was verified against nullptr. Check lines: 762, 770. freehand-base.cpp 762
  • V595 The 'release_connection' pointer was utilized before it was verified against nullptr. Check lines: 505, 511. gradient-toolbar.cpp 505
  • V595 The 'modified_connection' pointer was utilized before it was verified against nullptr. Check lines: 506, 514. gradient-toolbar.cpp 506


Пропущенная точка с запятой


Предупреждение PVS-Studio: V504 It is highly probable that the semicolon ';' is missing after 'return' keyword. svg-fonts-dialog.cpp 167

void GlyphComboBox::update(SPFont* spfont)
{
  if (!spfont) return // <=
//TODO: figure out why do we need to append("")
// before clearing items properly...

//Gtk is refusing to clear the combobox 
//when I comment out this line
  this->append(""); 
  this->remove_all();
}



После return пропущена точка с запятой (";"), что и является причиной проблемы, описанной в комментариях автора. Поскольку, если закомментировать строку:

 this->append("");

то получится конструкция вида:

if (!spfont) return this->remove_all();

Соответственно, combobox будет очищаться только в случае, когда spfont == NULL.

Неиспользуемый параметр


Предупреждение PVS-Studio: V763 Parameter 'new_value' is always rewritten in function body before being used. sp-xmlview-tree.cpp 259

void element_attr_changed(.... const gchar * new_value, ....)
{
  NodeData *data = static_cast<NodeData *>(ptr);
  gchar *label;

  if (data->tree->blocked) return;

  if (0 != strcmp (key, "id") &&
      0 != strcmp (key, "inkscape:label"))
        return;

  new_value = repr->attribute("id"); // <=
  ....
}

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

Аналогичная ситуация:
  • V763 Parameter 'widget' is always rewritten in function body before being used. ruler.cpp 923


Указатель на несуществующий массив


Предупреждение PVS-Studio: V507 Pointer to local array 'n' is stored outside the scope of this array. Such a pointer will become invalid. inkscape.cpp 582

void
Application::crash_handler (int /*signum*/)
{
  ....
  if (doc->isModifiedSinceSave()) {
    const gchar *docname;
  ....
  if (docname) {
    ....
    if (*d=='.' && d>docname && dots==2) {
      char n[64];
      size_t len = MIN (d - docname, 63);
      memcpy (n, docname, len);
      n[len] = '\0';
      docname = n;
    }
  }
  if (!docname || !*docname) docname = "emergency";
  ....
}

Массив n имеет время жизни меньше, чем время жизни указателя docname на него. Это приводит к работе с недействительным указателем docname. Одним из решений проблемы будет определение массива n рядом с указателем docname:

....
if (doc->isModifiedSinceSave()) {
  const gchar *docname;
  char n[64];
....

Аналогичные указатели:
  • V507 Pointer to local array 'in_buffer' is stored outside the scope of this array. Such a pointer will become invalid. inkjar.cpp 371
  • V507 Pointer to local array 'out_buffer' is stored outside the scope of this array. Such a pointer will become invalid. inkjar.cpp 375


Неверное имя объекта в условии


Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 640, 643. font-variants.cpp 640

void
FontVariants::fill_css( SPCSSAttr *css ) 
{
  ....
  if( _caps_normal.get_active() ) {
    css_string = "normal";
    caps_new = SP_CSS_FONT_VARIANT_CAPS_NORMAL;
  } else if( _caps_small.get_active() ) {
    ....
  } else if( _caps_all_small.get_active() ) {
    ....
  } else if( _caps_all_petite.get_active() ) { // <=
    css_string = "petite";                     // <=
    caps_new = SP_CSS_FONT_VARIANT_CAPS_PETITE;
  } else if( _caps_all_petite.get_active() ) { // <=
    css_string = "all-petite";                 // <=
    caps_new = SP_CSS_FONT_VARIANT_CAPS_ALL_PETITE;
  } 
  ....
}

В условии, идущем перед _caps_all_petite.get_active(), имя объекта должно быть _caps_petite, а не _caps_all_petite. Ошибка скорее всего произошла в результате Copy-Paste.

Неаккуратное использование числовых констант


Предупреждение PVS-Studio: V624 The constant 0.707107 is being utilized. The resulting value could be inaccurate. Consider using the M_SQRT1_2 constant from <math.h>. PathOutline.cpp 1198

void
Path::OutlineJoin (....)
{
  ....
  if (fabs(c2) > 0.707107) {
    ....
  }
  ....
}

Такая запись не совсем корректна и может привести к уменьшению точности вычислений. Лучше использовать математические константу M_SQRT1_2 (the inverse of the square root of 2), объявленную в файле <math.h>. Думаю, на практике здесь всё работает хорошо, но захотелось обратить внимание и на такой пример некрасивого кода.

Аналогичные предупреждения:
  • V624 The constant 1.414213562 is being utilized. The resulting value could be inaccurate. Consider using the M_SQRT2 constant from <math.h>. verbs.cpp 1848
  • V624 The constant 3.14159 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. odf.cpp 1568
  • V624 The constant 1.414213562 is being utilized. The resulting value could be inaccurate. Consider using the M_SQRT2 constant from <math.h>. inkscape-preferences.cpp 1334


Идентичные выражения


Предупреждение PVS-Studio: There are identical sub-expressions 'Ar.maxExtent() < tol' to the left and to the right of the '&&' operator. path-intersection.cpp 313
void mono_intersect(....)
{
   if(depth > 12 || (Ar.maxExtent() < tol && Ar.maxExtent() < tol)) 
   {
     ....
   }
   ....
}

Проверка условия Ar.maxExtent() < tol выполняется дважды. Скорее всего это получилось в результате внесения каких-то исправлений в код. Следует исправить выражение или просто убрать дублирующую проверку.

Аналогичная проверка:
  • V501 There are identical sub-expressions 'Ar.maxExtent() < 0.1' to the left and to the right of the '&&' operator. path-intersection.cpp 364


Одинаковые действия в блоках if и else


Предупреждение PVS-Studio: The 'then' statement is equivalent to the 'else' statement. ShapeRaster.cpp 1825

void Shape::AvanceEdge(....)
{
  ....
  if ( swrData[no].sens ) { 
    if ( swrData[no].curX < swrData[no].lastX ) {
      line->AddBord(swrData[no].curX,
                    swrData[no].lastX,
                    false);
    } else if ( swrData[no].curX > swrData[no].lastX ) { 
        line->AddBord(swrData[no].lastX,
                      swrData[no].curX,
                      false);
      }
  } else {
    if ( swrData[no].curX < swrData[no].lastX ) {
      line->AddBord(swrData[no].curX,
                    swrData[no].lastX,
                    false);
    } else if ( swrData[no].curX > swrData[no].lastX ) {
        line->AddBord(swrData[no].lastX,
                      swrData[no].curX,
                      false);
    }
  }
}

Код в блоках if и else одинаков, поэтому стоит просмотреть это место и либо исправить логику работы, либо удалить дублирующую ветку.

Аналогичные места:
  • V523 The 'then' statement is equivalent to the 'else' statement. ShapeRaster.cpp 1795
  • V523 The 'then' statement is equivalent to the 'else' statement. PathCutting.cpp 1323
  • V523 The 'then' statement is equivalent to the 'else' statement. ShapeSweep.cpp 2340


Заключение


В ходе проверки было выявлено немало ошибок, допущенных по невнимательности. Статический анализатор PVS-Studio может эффективно выявлять такие ошибки и тем самым экономить время и нервы программиста. Главное выполнять анализ кода регулярно, чтобы сразу выявлять опечатки и прочие недоработки. Разовые проверки, такая как эта, хотя хорошо рекламируют PVS-Studio, но малоэффективны. Рассматривайте сообщения от статического анализатора как расширенные предупреждения от компилятора. А с сообщениями компилятора надо работать постоянно, а не разово перед релизом. Надеюсь эта аналогия близка и понятна душе любого программиста, переживающего за качество кода.

Предлагаю скачать и попробовать PVS-Studio на своем собственном проекте.

P.S.




В нашей компании решили немного поэкспериментировать с Instagram. Не знаем получится ли из этого что-то, но приглашаю всех желающих последовать за "pvsstudio".


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Egor Bredikhin.Waiting for the Linux-version: Checking the Code of Inkscape Graphics Editor.
Прочитали статью и есть вопрос?
Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
Поделиться с друзьями
-->

Комментарии (48)


  1. chemtech
    15.08.2016 18:07
    #9751994

    Он скорее на Launchpad.net доступен.
    Как вы относитесь к тому что, разработчики не указывают в changelog что PVS-Studio нашел ошибки в их ПО?


    1. Andrey2008
      15.08.2016 19:41
      #9752150
      +4

      Нейтрально.


    1. A-Stahl
      15.08.2016 19:44
      #9752156
      +15

      >разработчики не указывают в changelog что PVS-Studio нашел
      А должны? Зачем?! Может ещё указать пиво какой марки предпочитает каждый из программистов, приложивших руку к коду?


      1. gmvbif
        15.08.2016 20:44
        #9752216
        +5

        А вот это интересный вариант. Может быть потом какие-нибудь корреляции выяснились между качеством кода и предпочитаемым напитком. HR ходили бы в бары хантить персонал.


      1. Newbilius
        16.08.2016 10:58
        #9752738

        Можно было бы давать бесплатную версию за рекламу инструмента в чейнджлоге и описании обновлений :-)


  1. Salabar
    15.08.2016 18:37
    #9752056

    А почему

    if (!spfont) return // <=

    вообще скомпилировалось? Там же void функция, которая не может ничего возвращать.


    1. hdfan2
      15.08.2016 18:55
      #9752074
      +5

      void функция может вернуть результат другой void функции.


      1. beeruser
        15.08.2016 23:46
        #9752332

        отсутствие результата тоже результат


  1. b-s-a
    15.08.2016 19:45
    #9752158
    +6

    Однопроходовый цикл while используется вместо кучи if () goto… Согласен, не совсем красивый код, но всяко лучше кучи goto.


    1. vagran
      15.08.2016 20:10
      #9752178
      +2

      Тоже часто пользуюсь подобной идиомой. Можно ещё do {} while(false), если условий нет. Странно, вроде как очень востребованный паттерн, а что-то не припомню ни в одном языке специальной конструкции для него. Например, было бы логично сделать просто do {} с возможностью break.


      1. springimport
        15.08.2016 20:35
        #9752208

        Как раз думал чем же заменить:

        $status = true;

if (1 == 2) {
    $status = false;
}

if ($status) {
    // ...
}

if ($status) {
    // ...
}


        1. b-s-a
          16.08.2016 11:39
          #9752816

          Лучше замени на функцию с return. Это более красивое решение.


          1. springimport
            16.08.2016 15:58
            #9753494

            Да, так и сделал, но это не решает другой проблемы. Предположим, если условие 2

            if ($status) {

            в случае неудачи должно что-то выполнить, то return не спасает.


            1. b-s-a
              16.08.2016 16:34
              #9753586

              Если честно, я вообще не вижу смысла в вашем коде. Так как вторая проверка статуса имеет смысл только внутри первой.
              А причем тут вообще while (cond) {… break; }?


              1. springimport
                16.08.2016 16:42
                #9753608
                +1

                Не только. Можно все сделать как вы сказали, но получится вот это:

                        if (1) {
            if (2) {
                if (3) {
                    if (4) {
                        if (5) {
                            
                        }
                    }
                }
            }
        }


                1. b-s-a
                  16.08.2016 16:46
                  #9753618
                  -1

                  Понимаю, код не красивый — сам такие конструкции стараюсь избегать. Но зато смысл работы очевидный, в отличие от первого варианта.


                  1. sys_int64
                    18.08.2016 06:08
                    #9756732

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


      1. symbix
        15.08.2016 21:00
        #9752236
        -1

        Зачем больше, если можно меньше?

        Вон в Go обходятся одним for на все случаи жизни. Аналогом такой конструкции будет

        for {
        //…
        break
        }


        1. monah_tuk
          16.08.2016 09:54
          #9752650

          Не совсем аналогом: while сначала проверяет условие и заходит в тело только если оно истинно, т.е. ваша конструкция будет выглядеть как-то так:


          if cond {
  for {
    // ..
    break
  }
}


        1. vagran
          16.08.2016 10:38
          #9752708

          Ну это такой же костыль, как while(true) { break;}. Желательна конструкция, в которой не надо будет в конце лишний break делать.


          1. monah_tuk
            17.08.2016 02:05
            #9754392

            Вы забыли про:


            do {

} while (false);

            Ставить break в конце не нужно ;-) Но в коде выше тогда потребуется отдельный if (cond) ...


            1. vagran
              17.08.2016 07:40
              #9754468

              Не забыл, в своём первом комментарии выше я про него написал. Если был бы do{}, то if лучше было бы ставить явно.


    1. Randl
      16.08.2016 10:35
      #9752702

      А чем break лучше goto? Я понимаю, что используя goto можно писать очень запутанный код, который сложно поддерживать, но тут-то использование абсолютно прозрачно.


      1. b-s-a
        16.08.2016 11:38
        #9752814
        +2

        С goto могут возникнуть проблемы с созданием переменных. Имхо, лучше выделить в отдельную функцию.


  1. vagran
    15.08.2016 20:09
    #9752176

    Del


  1. Crosshairs
    15.08.2016 20:27
    #9752192
    +2

    Здравствуйте! Хочу вас спросить: можно ли предложить вам проекты открытого ПО? Или вы проверяете только то, что сами сочтете нужным?
    К примеру, было бы очень неплохо проверить Mesa — недавно как раз вышла новая версия.


    1. Andrey2008
      15.08.2016 20:50
      #9752220

      Мы открыты предложениям. Пишите нам или можете оставлять комментарии здесь. Mesa возьмём на заметку.


      1. Salabar
        15.08.2016 22:18
        #9752296

        Посмотрите еще слои валидации для API Vulkan. Проект сам по себе небольшой, и вещи делает достаточно скучные, но от его качества зависит, по сути, будет ли этой библиотекой кто-то пользоваться.
        https://github.com/KhronosGroup/Vulkan-LoaderAndValidationLayers


  1. viktorious
    15.08.2016 22:25
    #9752302
    +7

    Самым очевидным решением проблемы является использование блока try {....} catch(const std::bad_alloc &) {....}

    Простите за занудство, но самым очевидным решением является замена new на new (std::nothrow). Тогда остальной код менять не надо — разве что бессмысленный delete[] buffer удалить.


  1. monah_tuk
    16.08.2016 10:08
    #9752662
    +3

    то данный метод определенно следует переписать так:

    Простите, но нет. Судя по коду автор явно хотел сделать триггер, что если значение back меняется, то только тогда делать ResetPoints(), т.е. код бы получился таким:


    void
Path::SetBackData (bool nVal)
{
  if (back != nVal) {
    back = nVal;
    ResetPoints();
  }
}

    Как минимум это подтверждается, если проверить случаи когда back == true и nVal == true или back == false и nVal == false — в этих случаях никаких действий код автора выполнять не будет.


    1. LifeKILLED
      16.08.2016 22:14
      #9754178

      Да уж, разобраться в том месиве if и правда сложновато, наверняка, даже написавший её программист боится трогать ту функцию, работает — и слава Богу…

      оффтоп
      Мне больше нравится в стиле Кармака:

      void
Path::SetBackData (bool nVal)
{
  if (back == nVal) return;
    
  back = nVal;
  ResetPoints();
}


      1. monah_tuk
        17.08.2016 02:01
        #9754388

        разобраться в том месиве if и правда сложновато

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


        оффтоп

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


  1. Randl
    16.08.2016 10:38
    #9752710

    //TODO: figure out why do we need to append("")
// before clearing items properly...

    К вопросу о том, что статический анализ не нужен, потому что "как можно не заметить отсутсвие точки с запятой?!!"


    1. rkfg
      16.08.2016 14:13
      #9753206
      +1

      Если писать без автоформатирования, то да, возможно. Я привык часто жать Ctrl+Shift+F, чтобы код выглядел красиво, в этом случае выражение прыгнуло бы к return и стало бы самоочевидно. Ошибка с пропущенной запятой точно так же стала бы заметна.

      А ещё я совершенно не понимаю, зачем в проекте на C++ использовать сишные strcmp и memcpy, которые известны букетами ошибок. Если даже аргумент приходит в (const) char *, безопаснее сначала сделать из него std::string и после этого сравнивать с чем угодно без глупых подсчётов числа букв.


      1. Randl
        16.08.2016 14:27
        #9753254
        +1

        Лично у меня стоит автоформатирование при коммите, поэтому мне, если TODO был написал в одном коммите с самим кодом, автоформатирование не помогло бы.


        Если даже аргумент приходит в (const) char *

        Ждем-с string_view


        1. rkfg
          16.08.2016 14:34
          #9753264

          В C++17 внесён, можно уже не ждать. Если категорически нужна совместимость со старым рантаймом, можно статически слинковаться с libstdc++. В любом случае, вряд ли копирование в std::string стало бы внезапно бутылочным горлом в этой функции, а профайлер прогонять всяко надо время от времени. Читабельность и надёжность кода же повысится в разы.

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


  1. Portnov
    16.08.2016 14:13
    #9753208

    А эти результаты сообщены разработчикам inkscape?


    1. EgorBredikhin
      16.08.2016 14:13
      #9753216

      Да, конечно.


  1. tasmail
    16.08.2016 14:13
    #9753210
    -2

    Со своим уставом в чужой монастырь.


  1. gold_user
    16.08.2016 14:13
    #9753212
    -2

    Про PVS-Studio vs Clang Static Analyzer читал уже неоднократно. Равно как и про то, как вы постоянно проверяете open source проекты. Какой смысл в этих проверках? В феврале вы проверяли FreeBSD, но что-то ваша работа незаметна на графике ошибок и их устранении.
    PVS-Studio работает только под Windows и только с Visual Studio. Тогда чем вам не угодил open source? Да, open source с ошибками, а где их нет? На чужом несчастье счастья своего не построишь. Слышали такое?

    After analyzing over 10 billion lines of code through Coverity Scan, the new 2014 Coverity Scan Open Source Report explains how:
    Commercial code is more compliant to security standards than open source code
    Defect density (defects per 1,000 lines of code) of open source code and commercial code has continued to improve since 2013
    OpenSSL utilized Coverity Scan during their post-Heartbleed investigation
    Early adoption of complimentary tools addressing legacy and newly written code is now truly a necessity
    A responsible shift in best practices by open source leaders such as Linux, LibreOffice, NetBSD, and Apache Hadoop are helping to improve the general state of all open source software –highlighted by the improvements found in defect density from 2013 to 2014


    Что касаемо inkscape, то его проверяют регулярно: https://scan.coverity.com/projects/inkscape

    И заключительный вопрос: чем же ваша PVS-Studio интереснее Coverity?


    1. Andrey2008
      16.08.2016 14:31
      #9753258

      Про PVS-Studio vs Clang Static Analyzer читал уже неоднократно.


      Да, было такое. А скоро я и GCC «размажу по стенке» :). Статья уже есть. Но перед ней в очереди ещё несколько, так что придётся подождать.


      Какой смысл в этих проверках?


      Смысл проверок — реклама PVS-Studio. Благодаря её, у нас появляются новые клиенты, многие из которых используют PVS-Studio вот уже в течении нескольких лет.


      В феврале вы проверяли FreeBSD, но что-то ваша работа незаметна на графике ошибок и их устранении.


      И не должна. Мы всегда говорим, что разовые проверки не имеют смысла и годятся только как реклама. Смысл статического анализа в регулярном использовании. Вот если его ежедневного год использовать, тогда можно будет заметить вклад.


      PVS-Studio работает только под Windows и только с Visual Studio.


      Это скоро изменится.


      Тогда чем вам не угодил open source? Да, open source с ошибками, а где их нет? На чужом несчастье счастья своего не построишь. Слышали такое?


      Эээ..


      Что касаемо inkscape, то его проверяют регулярно: https://scan.coverity.com/projects/inkscape


      Отлично. Это значит, что мы легко находим ошибки уже после Coverity. Наш анализатор становится всё круче и круче.


      И заключительный вопрос: чем же ваша PVS-Studio интереснее Coverity?


      У PVS-Studio лучше соотношение эффективность/цена.


      1. gold_user
        16.08.2016 19:46
        #9753952

        А падать не больно будет?


      1. LifeKILLED
        16.08.2016 22:05
        #9754166

        Кстати, о Coverty. А не думали так же проверять анализаторы? Вряд ли у них открыты исходники, но получилось бы крайне иронично.


    1. gmvbif
      16.08.2016 14:49
      #9753294

      А как проверка с помощью PVS-Studio должна была быть заметна на сайте Coverty?


  1. Satus
    16.08.2016 14:13
    #9753214
    +2

    По приведённым ошибкам сложилось впечатление, что код писали программисты на С, которых зачем-то заставили писать на С++.


  1. JerleShannara
    16.08.2016 18:01
    #9753808
    +1

    В ожидании линукса — есть «свободная» реализация Bios/UEFI для 100+ материнских плат по имени CoreBoot. Проект периодически проверяется Coverity. Есть ли в планах проверка такой экзотики?


    1. Andrey2008
      16.08.2016 19:33
      #9753938

      Не мы, но проверяли: Проверяем открытый исходный код UEFI для Intel Galileo при помощи PVS-Studio. Или это другое? Я в этой теме не ориентируюсь.


      1. JerleShannara
        16.08.2016 20:12
        #9753976

        Это другое — http://coreboot.org, к проверенному там куском относится возможность собрать этот самый UEFI в качествe payload-а.