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

Введение

CMake — это инструмент, который помогает создавать проекты для разных платформ и компиляторов. Он не собирает код сам напрямую, а вместо этого работает как посредник, который преобразует правила вашего проекта в готовые файлы для других программ-сборщиков, таких как GNU make, Ninja, MSBuild и т.д.

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

Я собирала проект из исходного кода вот по этой инструкции. Хотелось бы отметить, что проблем при генерации не возникло, за что спасибо нашему герою. Если вам интересно узнать более детально, как работает CMake, можете ознакомиться с документацией.

После сборки я открыла .sln файл и проверила плагином PVS-Studio. Тег, на котором был зафиксирован репозиторий – v4.1.0. Отмечу, когда-то очень давно мы уже проверяли нашего героя. Кому интересно, предлагаю ознакомиться с прошлой статьёй.

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

Срабатывания

Фрагмент N1

Предупреждение PVS-Studio: V1061 Extending the 'std' namespace may result in undefined behavior. cmList.h 1322

namespace std // <=
{
  inline void swap(cmList& lhs, cmList& rhs) noexcept
  {
    lhs.swap(rhs); 
  }
}

Поведение C++ программы не определено, если в ней добавляются новые функции в пространство имён std. Как видно в примере, новая перегрузка функции std::swap была добавлена внутрь этого пространства.

Как же мы можем это исправить? Правильный способ реализовать эту функцию для пользовательского типа — объявить её в том же пространстве имён, что и сам тип:

class cmList
{
public:
  void swap(cmList& other) noexcept { /* implementation */ }
private:
  /* private data members */
};

inline void swap(cmList& lhs, cmList& rhs) noexcept
{
  lhs.swap(rhs);
}

Хорошо, а как же компилятор будет понимать, что нужно достать именно эту функцию? Для этого разработчику стоит вызывать функцию с помощью неквалифицированного имени:

template <typename T> 
void foo(T &obj1, T &obj2)
{ 
  using std::swap;  // сделать std::swap доступной в этой области видимости
  ....
  swap(obj1, obj2); // вызвать лучший вариант swap для объектов типа T 
  ....
}

В этом примере using std::swap; делает функцию std::swap доступной в текущей области видимости, а при неквалифицированном вызовe swap(obj1, obj2); компилятор выберет наиболее подходящую перегрузку через механизм argument-dependent lookup (ADL). Если в нашем примере шаблонный параметр T будет специализирован типом cmList, то компилятор поищет перегрузку swap также и в области определения этого класса и найдёт там более специализированную версию.

Фрагмент N2

void cmLocalGenerator::GetDeviceLinkFlags(
  cmLinkLineDeviceComputer& linkLineComputer, 
  std::string const& config,
  std::string& linkLibs, 
  std::string& linkFlags, 
  std::string& frameworkPath,
  std::string& linkPath, 
  cmGeneratorTarget* target)
{
  cmGeneratorTarget::DeviceLinkSetter setter(*target);

  cmComputeLinkInformation* pcli = target->GetLinkInformation(config);

  auto linklang = linkLineComputer.GetLinkerLanguage(target, config);
  auto ipoEnabled = target->IsIPOEnabled(linklang, config);
  if (!ipoEnabled) {
    ipoEnabled = linkLineComputer.ComputeRequiresDeviceLinkingIPOFlag(*pcli);
  }
  ....
  if (pcli) {
    this->OutputLinkLibraries(pcli,
                              &linkLineComputer, 
                               linkLibs, 
                               frameworkPath,linkPath);
  }
  ....
}

Видите ошибку? При быстром взгляде она заметна не сразу. В этом фрагменте кода указатель pcli разыменовывается при вызове функции ComputeRequiresDeviceLinkingIPOFlag до того, как проверяется на валидность. Если pcli в реальности окажется нулевым, то поведение будет не определено. Но пример не так очевиден, как кажется на первый взгляд. Мы решили посмотреть, присутствовала ли ошибка с самого начала, и ответ — нет:

cmComputeLinkInformation* pcli = target->GetLinkInformation(config);
const std::string linkLanguage =
  linkLineComputer->GetLinkerLanguage(target, config);

if (pcli)
{
  // Compute the required cuda device link libraries when
  // resolving cuda device symbols
  this->OutputLinkLibraries(pcli, linkLineComputer, linkLibs,
                            frameworkPath, linkPath);
}

Разработчик подразумевал, что указатель pcli может быть нулевым, и защитился проверкой. Затем в коммите 96bc59b1 другой разработчик добавил новую логику, но при её вставке забыл учесть этот факт. И здесь статический анализ мог бы показать себя с лучшей стороны: при отправке Merge Request код может пройти автоматическую проверку изменений.

Самый простой вариант исправления проблемы:

auto linklang = linkLineComputer.GetLinkerLanguage(target, config);
auto ipoEnabled = target->IsIPOEnabled(linklang, config);
if (!ipoEnabled && pcli) {
  ipoEnabled = linkLineComputer.ComputeRequiresDeviceLinkingIPOFlag(*pcli);
}
....

А вот как PVS-Studio предупреждает об этой ошибке: V595 The 'pcli' pointer was utilized before it was verified against nullptr. Check lines: 1445, 1454. cmLocalGenerator.cxx 1445

Схожие срабатывания:

• V595 The 'this->Properties[index]' pointer was utilized before it was verified against nullptr. Check lines: 904, 906. cmCTestMultiProcessHandler.cxx 904

• V595 The 'cp->Commands' pointer was utilized before it was verified against nullptr. Check lines: 536, 539. ProcessWin32.c 536

• V595 The 'copy_rule' pointer was utilized before it was verified against nullptr. Check lines: 3004, 3006. cmLocalGenerator.cxx 3004V

Фрагмент N3

Предупреждение PVS-Studio: V573 Uninitialized variable 'intDir' was used. The variable was used to initialize itself. cmGlobalVisualStudio7Generator.cxx 626

std::string cmGlobalVisualStudio7Generator::WriteUtilityDepend(
  cmGeneratorTarget const* target)
{
  std::vector<std::string> configs =
    target->Target->GetMakefile()->GetGeneratorConfigs(
      cmMakefile::ExcludeEmptyConfig);
  ....
  /* clang-format on */
  std::string intDirPrefix =
    target->GetLocalGenerator()->MaybeRelativeToCurBinDir(
      cmStrCat(target->GetSupportDirectory(), '\\'));
  for (std::string const& i : configs) {
    std::string intDir = cmStrCat(intDir, i);               // <=
    ....
  }
  ....
}

Интересный баг, который попал в основную ветку в коммите b82a74d9. Разработчик перебирает строки из контейнера configs и формирует путь до промежуточной директории (intDir), которая будет записана в *.vcproj файл.

Проблема заключается в том, что переменная intDir участвует в инициализации самой же себя. Поведение в этом случае будет не определено, т.к. время жизни объекта начнётся только после завершения инициализации ([1], [2]). Можно заметить, что перед циклом формируется переменная с похожим именем и постфиксом Prefix, и, скорее всего, она и должна быть использована при инициализации intDir:

std::string intDirPrefix = ....;
for (std::string const& i : configs) {
  std::string intDir = cmStrCat(intDirPrefix, i);
  ....
}

Фрагмент N4

Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'writeOp' variable should be used instead of 'readOp'. cmDebuggerWindowsPipeConnection.cxx 20

class DuplexPipe_WIN32
{
public:
  DuplexPipe_WIN32(HANDLE read);
  ....
private:
  HANDLE hPipe;
  OVERLAPPED readOp;
  OVERLAPPED writeOp;
};

DuplexPipe_WIN32::DuplexPipe_WIN32(HANDLE pipe) : hPipe(pipe)
{
  readOp.Offset = readOp.OffsetHigh = 0;
  readOp.hEvent = CreateEvent(NULL, true, false, NULL);
  writeOp.Offset = readOp.OffsetHigh = 0;
  writeOp.hEvent = CreateEvent(NULL, true, false, NULL);
}

В теле конструктора DuplexPipe_WIN32 происходит инициализация полей readOp и writeOp типа OVERLAPPED. В этой структуре разработчик инициализирует поля Offset, OffsetHigh, hEvent. Начал он с поля readOp, затем настала очередь writeOp, и для его инициализации он решил скопировать две строки выше и поменять название.

По невнимательности разработчик забыл поменять также readOp.OffsetHigh, что может стать причиной не очень забавных багов. Кажется, что исправление очевидно для всех, однако можно сделать несколько другой вариант:

class DuplexPipe_WIN32
{
public:
  DuplexPipe_WIN32(HANDLE read);
  ....
private:
  HANDLE hPipe;
  OVERLAPPED  readOp {};
  OVERLAPPED writeOp {};
};

DuplexPipe_WIN32::DuplexPipe_WIN32(HANDLE pipe): hPipe(pipe)
{
  readOp.hEvent = CreateEvent(NULL, true, false, NULL);
  writeOp.hEvent = CreateEvent(NULL, true, false, NULL);
}

Согласно документации:

All of the members of the OVERLAPPED structure must be initialized to zero unless an event will be used to signal completion of an I/O operation. If an event is used, the hEvent member of the OVERLAPPED structure specifies a handle to the allocated event object.

Инициализаторы полей по умолчанию в определении класса как раз заполнят всё нулями в OVERLAPPED, а затем в теле конструктора произойдёт инициализация readOp.hEvent и writeOp.hEvent.

Фрагмент N5

Предупреждение PVS-Studio: V1028 Possible overflow. Consider casting operands of the '_yybytes_len + 2' operator to the 'yy_size_t' type, not the result. cmExprLexer.cxx 1851

typedef void* yyscan_t;
typedef size_t yy_size_t;

void *yyalloc ( yy_size_t , yyscan_t yyscanner );

YY_BUFFER_STATE yy_scan_bytes  (const char * yybytes,
                                int  _yybytes_len ,
                                yyscan_t yyscanner)
{
  YY_BUFFER_STATE b;
  char *buf;
  yy_size_t n;
  int i;

  // Get memory for full buffer, including space for trailing EOB's.
  n = (yy_size_t) (_yybytes_len + 2);
  buf = (char *) yyalloc( n , yyscanner );
  if ( ! buf )
  YY_FATAL_ERROR( "out of dynamic memory in yy_scan_bytes()" );

  for ( i = 0; i < _yybytes_len; ++i )
    buf[i] = yybytes[i];

  buf[_yybytes_len] = buf[_yybytes_len + 1] = YY_END_OF_BUFFER_CHAR;
  ....
}

Переменная yybytes_len имеет тип int и участвует в формировании нового динамически аллоцированного буфера buf. Для этого вычисляется его размер в виде знакового выражения, и результат конвертируется в беззнаковый. При сложении может произойти знаковое переполнение (поведение которого не определено), если yybytes_len находится в диапазоне [INT_MAX – 1 .. INT_MAX]. Результат преобразования может получиться совсем не тем, что ожидал разработчик.

Вообще, описанная проблема подпадает под категорию 64-битных ошибок. И, к сожалению, чтобы решить её грамотно, нужно менять интерфейс у этой функции: параметр размера буфера должен быть объявлен с типом size_t, а не int. Причём эта функция уже вызывается с аргументом типа size_t, который сконвертировали в int:

YY_BUFFER_STATE yy_scan_string (const char * yystr ,
                                yyscan_t yyscanner)
{
  return yy_scan_bytes( yystr, (int) strlen(yystr) , yyscanner);
}

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

Фрагмент N6

/**
 * Append two or more strings and produce new one.
 * Programmer must 'delete []' the resulting string,
 * which was allocated with 'new'.
 * Return 0 if inputs are empty or there was an error
 */
char* SystemTools::AppendStrings(char const* str1,
                                 char const* str2)
{
  if (!str1) {
    return SystemTools::DuplicateString(str2);
  }
  if (!str2) {
    return SystemTools::DuplicateString(str1);
  }
  size_t len1 = strlen(str1);
  char* newstr = new char[len1 + strlen(str2) + 1];
  if (!newstr) {
    return nullptr;
  }
  strcpy(newstr, str1);
  strcat(newstr + len1, str2);
  return newstr;
}

Перед нами функция SystemTools::AppendStrings, которая призвана объединить две переданные строки в новом буфере и вернуть указатель на него.

Что же должно случиться, если при попытке выделить память произойдёт ошибка? Обратим внимание на комментарий к функции: "Если произошла какая-то ошибка, то функция должна вернуть нулевой указатель". Так ли это будет на самом деле? Нет, выражение new в случае ошибки бросит исключение std::bad_alloc. Более того, из-за этого следующая проверка не имеет смысла: если броска исключения не произошло, выражение new вернёт валидный указатель.

Чтобы исправить проблему, нужно вызвать nothrow-версию аллоцирующей функции:

char* newstr = new (std::nothrow) char[len1 + strlen(str2) + 1];
if (!newstr) {
  return nullptr;
}

Вот так анализатор сообщает об этой проблеме: V668 There is no sense in testing the 'newstr' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SystemTools.cxx 1724

Схожее срабатывание:

• V668 There is no sense in testing the 'newstr' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SystemTools.cxx 1747

Фрагмент N7

Предупреждение PVS-Studio: V557 Array overrun is possible. The value of 'length - 1' index could reach 18446744073709551615. ProcessWin32.c 1942

void kwsysProcessCleanErrorMessage(kwsysProcess* cp)
{
  /* Remove trailing period and newline, if any. */
  size_t length = strlen(cp->ErrorMessage);
  if (cp->ErrorMessage[length - 1] == '\n') {
    cp->ErrorMessage[length - 1] = 0;
    --length;
    if (length > 0 && cp->ErrorMessage[length - 1] == '\r') {
      cp->ErrorMessage[length - 1] = 0;
      --length;
    }
  }
  if (length > 0 && cp->ErrorMessage[length - 1] == '.') {
    cp->ErrorMessage[length - 1] = 0;
  }
}

Утилитная функция призвана сделать небольшие преобразования в сообщении об ошибке. Если сообщение в конце содержит последовательность символов .\r\n, то функция удалит их. Однако перед проверкой, что последний символ равен \n, разработчик не проверил длину сообщения. Если оно пришло пустое, то при вычислении индекса произойдёт беззнаковое переполнение, итоговое значение будет равно SIZE_MAX и произойдёт гарантированный выход за границу буфера. Поведение такой операции не определено. Занятно, что в конце функции при удалении символа . разработчик необходимую проверку уже сделал.

Исправленный код:

void kwsysProcessCleanErrorMessage(kwsysProcess* cp)
{
  /* Remove trailing period and newline, if any. */
  size_t length = strlen(cp->ErrorMessage);
  if (length > 0 && cp->ErrorMessage[length - 1] == '\n')
  ....
}

Фрагмент N8

Предупреждение PVS-Studio: V1043 A global object variable 'cmPropertySentinel' is declared in the header. Multiple copies of it will be created in all translation units that include this header file. cmStatePrivate.h 27

// cmStatePrivate.h
....
static std::string const cmPropertySentinel = std::string();

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

• она не volatile, но const-квалифицирована;

• она объявлена со спецификатором static.

При беглом взгляде на кодовую базу было обнаружено несколько включений этого заголовочного файла:

• cmState.cxx

• cmStateDirectory.cxx

• cmStateSnapshot.cxx

Поправить это можно двумя способами в зависимости от версии стандарта, применяемого в проекте.

До C++17:

// cmStatePrivate.h
....
extern std::string const cmPropertySentinel;

// some translation unit

#include "cmStatePrivate.h"

std::string const cmPropertySentinel = std::string();

Начиная с C++17:

// cmStatePrivate.h
....
inline std::string const cmPropertySentinel = std::string();

Фрагмент N9

Предупреждение PVS-Studio: V571 Recurring check. This condition was already verified in line 2528. cmQtAutoGenInitializer.cxx 2529

bool cmQtAutoGenInitializer::GetQtExecutable(
  GenVarsT& genVars,
  std::string const& executable, 
  bool ignoreMissingTarget) const
{
   ....
   std::string err;
   genVars.ExecutableFeatures = 
     this->GlobalInitializer->GetCompilerFeatures(
                                executable, genVars.Executable,
                                err, this->MultiConfig,
                                this->UseBetterGraph);

   if (this->MultiConfig && this->UseBetterGraph)
   {
    for (auto const& config : this->ConfigsList)
    {
      if (!genVars.ExecutableFeatures.Config[config])   // <=
      {
        if (!genVars.ExecutableFeatures.Config[config]) // <=
        {
          print_err(err);
          return false;
        }
      }
    }
  } 
  .... 
}

Анализатор выявил возможную избыточную проверку: условие !genVars.ExecutableFeatures.Config[config] проверяется дважды подряд. Давайте разбираться. Для начала посмотрим все необходимые типы и определения:

class CompilerFeatures { /* .... */ };

using CompilerFeaturesHandle = std::shared_ptr<CompilerFeatures>;

class cmQtAutoGen
{
public:
  ....
  /** String values with per configuration variants.  */
  template <typename C>
  class ConfigStrings
  {
  public:
    C Default;
    std::unordered_map<std::string, C> Config;
  };
  ....
};

class cmQtAutoGenInitializer : public cmQtAutoGen
{
public:
  /** Abstract moc/uic/rcc generator variables base class.  */
  struct GenVarsT
  {
    ....
    ConfigStrings<CompilerFeaturesHandle> ExecutableFeatures;
  };
  ....
private:
  ....
  bool GetQtExecutable(GenVarsT& genVars,
                       std::string const& executable,
                       bool ignoreMissingTarget) const;
  ....
};

Поле Config представляет собой контейнер типа std::unordered_map<std::string, std::shared_ptr<CompilerFeatures>>. При использовании оператора [] контейнер произведёт поиск элемента по заданному ключу и либо вернёт ссылку на уже существующий ранее объект, либо на новый, созданный по умолчанию. Условие будет вычислено как true в двух случаях:

1. Элемент присутствовал в ассоциативном контейнере и был нулевым.

2. Элемент отсутствовал в ассоциативном контейнере. Тогда внутри него будет создан новый нулевой умный указатель с заданным ключом.

При этом между двумя поисками элемента не происходит его непосредственной модификации. Не похоже, что этот код выполняется каким-либо образом многопоточно, т.к. в функции отсутствуют механизмы синхронизации. Очень похоже на то, что эта часть кода должна была проверить результат функции GetCompilerFeatures: все объекты по ключам из ConfigsList должны присутствовать внутри контейнера genVars.ExecutableFeatures и быть ненулевыми. Если что-то из этого нарушено, то функция должна залоггировать ошибку и вернуть false.

На основе этой идеи упростим код:

if (this->MultiConfig && this->UseBetterGraph)
{
  bool ok = std::all_of(
              this->ConfigsList.begin(),
              this->ConfigsList.end(),
              [&Config = genVars.ExecutableFeatures.Config](auto &&config)
              {
                auto it = Config.find(config);
                return it != Config.end() && it->second;
              });

  if (!ok)
  {
    print_err(err);
    return false;
  }
}

Схожее срабатывание:

• V571 Recurring check. The '!cm->SourceFile.empty()' condition was already verified in line 580. cmCTestBuildHandler.cxx 581

Фрагмент N10

Предупреждение PVS-Studio: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'arguments.ErrorVariable.empty()' and '!arguments.ErrorVariable.empty()'. cmExecuteProcessCommand.cxx 257

bool cmExecuteProcessCommand(std::vector<std::string> const& args,
                             cmExecutionStatus& status)
{
  ....
  else if (  arguments.ErrorVariable.empty() ||
             (!arguments.ErrorVariable.empty() &&
               arguments.OutputVariable != arguments.ErrorVariable))
  {
    ....  
  }
  ....
}

Анализатор сообщает, что логическое выражение содержит избыточную проверку. Выражение arguments.ErrorVariable.empty() дважды участвует в вычислениях, при этом второй раз оно инвертировано. Если это выражение в качестве левого операнда оператора || вычислится как false, то второй раз при вычислении правого операнда оно гарантированно будет true.

Упростим логическое выражение:

else if (   arguments.ErrorVariable.empty() 
         || arguments.OutputVariable != arguments.ErrorVariable)
{
  ....
}

Доказательство эквивалентности логических выражений

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

arguments.ErrorVariable.empty() как выражение A;

arguments.OutputVariable != arguments.ErrorVariable как выражение B.

Как можно заметить, последние два столбца идентичны, а значит два результирующих выражения эквивалентны при всех возможных комбинациях значений подвыражений. Таким образом, упрощённое выражение сохраняет ту же логику, что и исходное. Только упрощённый вариант проще для восприятия.

Схожие срабатывания:

• V728 An excessive check can be simplified. The '(A && B) || (!A && !B)' expression is equivalent to the 'bool(A) == bool(B)' expression. cmGlobalGhsMultiGenerator.cxx 341

• V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. testDynamicLoader.cxx 81

• V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. testDynamicLoader.cxx 88

• V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. testDynamicLoader.cxx 95

Фрагмент N11

Предупреждение PVS-Studio: V501 There are identical sub-expressions '!this->MakefileVariableSize' to the left and to the right of the '&&' operator. cmLocalUnixMakefileGenerator3.cxx 1292

class cmLocalUnixMakefileGenerator3 : public cmLocalCommonGenerator
{
....
private:
  int MakefileVariableSize;
....
};

std::string cmLocalUnixMakefileGenerator3::CreateMakeVariable(
  std::string const& s, 
  std::string const& s2)
{
  std::string unmodified = cmStrCat(s, s2);
  if ((!this->MakefileVariableSize && // <=
        unmodified.find('.') == std::string::npos) &&
      (!this->MakefileVariableSize &&
        unmodified.find('+') == std::string::npos) &&
      (!this->MakefileVariableSize &&
        unmodified.find('-') == std::string::npos)){
      return unmodified;
  }
}

В функции cmLocalUnixMakefileGenerator3::CreateMakeVariale присутствует избыточное повторение условия !this->MakefileVariableSize в логическом выражении. Это условие повторяется трижды перед каждой из проверок наличия символов ., +, - в строке unmodified. Такое повторение делает код менее читаемым и увеличивает его объём. Поскольку условие !this->MakefileVariableSize является общим для всех трёх проверок, его можно вынести за скобки, объединив все проверки в одно логическое выражение:

 if (!this->MakefileVariableSize &&
      unmodified.find('.') == std::string::npos &&
      unmodified.find('+') == std::string::npos &&
      unmodified.find('-') == std::string::npos) {
       return unmodified;
}

Мы убрали одно и то же подвыражение, однако в условии происходят три поиска символов в строке. Можно пойти ещё чуть дальше и провести микрооптимизацию. Заменим вызов функции-члена find на find_first_of, передав в строковом литерале все три символа:

if (!this->MakefileVariableSize && 
    unmodified.find_first_of(".+-") == std::string::npos)
{ 
  return unmodified;
}

Схожее срабатывание:

• V501 There are identical sub-expressions '!options.empty()' to the left and to the right of the '||' operator. cmVisualStudio10TargetGenerator.cxx 2912

Фрагмент N12

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the subsequent code fragment. cmComputeLinkInformation.cxx 1748

bool cmComputeLinkInformation::CheckImplicitDirItem(LinkEntry const& entry)
{
  BT<std::string> const& item = entry.Item;

  // We only switch to a pathless item if the link type may be
  // enforced.  Fortunately only platforms that support link types
  // seem to have magic per-architecture implicit link directories.
  if (!this->LinkTypeEnabled) {
    return false;
  }

  // Check if this item is in an implicit link directory.
  std::string dir = cmSystemTools::GetFilenamePath(item.Value);
  if (!cm::contains(this->ImplicitLinkDirs, dir)) {
    // Only libraries in implicit link directories are converted to
    // pathless items.
    return false;
  }

  // Only apply the policy below if the library file is one that can
  // be found by the linker.
  std::string file = cmSystemTools::GetFilenameName(item.Value);
  if (!this->ExtractAnyLibraryName.find(file)) {
    return false;
  }

  return false;
}

Анализатор намекает, что с функцией CheckImplicitDirItem что-то определённо не так:

• then-ветка последней конструкции if совпадает с нижележащим кодом (return false;);

• все ветви выполнения функции заканчиваются возвратом значения false;

• её вызов в функции AddFullItem никогда не приведёт к раннему возврату;

• тело функции можно целиком заменить на return false;, т.к. это не изменит наблюдаемого поведения программы.

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

В нашем примере можно предположить, что наиболее "положительный" результат функции — это прохождение объектом типа LinkEntry всех необходимых проверок с возвратом значения true.

Вариант исправленного кода:

....
std::string file = cmSystemTools::GetFilenameName(item.Value);
if (!this->ExtractAnyLibraryName.find(file)) {
  return false;
}

return true;

Схожие срабатывания:

• V523 The 'then' statement is equivalent to the subsequent code fragment. cmGlobVerificationManager.cxx 128

• V523 The 'then' statement is equivalent to the subsequent code fragment. cmListCommand.cxx 92

Заключение

На этом этапе наш рассказ о CMake подходит к концу. Этот герой, несмотря на найденные нами ошибки (от copy-paste до избыточных проверок), остаётся отличным инструментом. Мы все допускаем промахи, но это не делает нас плохими. Так и CMake — проект, который активно развивается, а его ошибки оперативно устраняются. Многие из ранее обнаруженных багов уже исправлены, и это подтверждает то, что CMake по-прежнему является надёжным и незаменимым помощником в мире разработки.

Если у вас есть свои проекты на C, C++, C#, Java, то вы можете проверить их нашим анализатором.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Elizaveta Zhegalova. CMake: hero who tripped over 12 bugs.