# CMake: hero who tripped over 12 bugs

Developers wield an entire army of tools, each promising to make their lives easier\. Yet hidden among this vast crowd stands a true hero—the one who comes to the rescue in the toughest battles\. Nomen illis CMake\! However, we all know that nothing is perfect, and even the strongest have their Achilles' heel\. Today, we'll spot what weaknesses lie behind the shining armor of our hero\.

![1277_CMake_v4_1_0/image1.png](https://import.viva64.com/docx/blog/1277_CMake_v4_1_0/image1.png)

## Intro

CMake is a tool that helps devs create projects for different platforms and compilers\. It doesn't build the code directly but works as an intermediary that converts the project rules into ready\-to\-use files for other build programs such as GNU make, Ninja, MSBuild, etc\. 

Our hero sets the rhythm and direction, ensuring a smooth workflow of all these components\. Thanks to CMake, developers can stay focused on writing code instead of wrestling with platform\-specific build quirks\. The program brings order to the chaotic build process, giving full freedom to choose tools for specific tasks\.

I built the project from source following this [guide](https://github.com/Kitware/CMake?tab=readme-ov-file#building-cmake-with-cmake)\. There were no problems during generation, and I owe that to our hero\. For curious dev knights, I leave the full [documentation](https://cmake.org/documentation/) to dive a little bit deeper on how to work with CMake\. 

After building, I opened the `.sln` file and checked it with the [PVS\-Studio](https://marketplace.visualstudio.com/items?itemName=EvgeniyRyzhkov.PVS-Studio) plugin\. The repository was committed at the [v4\.1\.0](https://gitlab.kitware.com/cmake/cmake/-/tree/v4.1.0?ref_type=tags) tag\. By the way, we had crossed paths with our hero before\. For curious devs, I suggest you read the previous [article](https://pvs-studio.com/en/blog/posts/cpp/0658/)\.

I filtered the results by High and Medium severity and included the general\-purpose warnings\. Well, all preparations have been completed\. Now it's time to challenge our hero\!

## Warnings

**Fragment N1**

The PVS\-Studio warning: [V1061](https://pvs-studio.com/en/docs/warnings/v1061/) Extending the 'std' namespace may result in undefined behavior\. [cmList\.h 1322](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmList.h#L1322)

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

The C\+\+ program will have [undefined](https://timsong-cpp.github.io/cppwp/n4950/namespace.std#1) behavior if developers add new functions to the `std` namespace\. As we can see in the example, devs have added a new overload of `std::swap` directly into that namespace\.

So, how can we fix it? The correct way to implement this function for a user\-defined type is to declare it in the same namespace as the type itself:

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

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

Another question: how will the compiler spot to get this particular function? Developers should call the function using an unqualified name:

```cpp
template <typename T> 
void foo(T &obj1, T &obj2)
{ 
  using std::swap;  // make 'std::swap' available in this scope
  ....
  swap(obj1, obj2); // call the best 'swap' for objects of the 'T' type 
  ....
}
```

In this example, `using std::swap;` brings the `std::swap` function into the current scope\. When `swap(obj1, obj2);` is unqualifiedly called, the compiler will select the most appropriate overload via the [argument\-dependent lookup](https://timsong-cpp.github.io/cppwp/n4950/basic.lookup.argdep) \(ADL\) mechanism\. If the `T` template parameter is specialized as the `cmList` type, the compiler will look for the `swap` overload also in this class namespace, and select it as the most appropriate version\.

**Fragment N2**

```cpp
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);
  }
  ....
}
```

See the error? At first glance, it's easy to miss\. The `pcli` pointer is dereferenced when the `ComputeRequiresDeviceLinkingIPOFlag` function is called, before it's checked for validity\. If `pcli` turns out to be `null`, it leads to [undefined](https://pvs-studio.com/en/blog/posts/cpp/0306/) behavior\. But the situation isn't as easy\-peasy as it looks\. We've dug a little deeper to see whether this bug has been there from the start—[the answer is no](https://gitlab.kitware.com/cmake/cmake/-/blob/4248bb452a5886ec4b632e2d602fcaafd2b13aaf/Source/cmLocalGenerator.cxx#L1432-1441):

```cpp
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);
}
```

Developers know the `pcli` pointer could be null and wisely guard themselves by checking it\. But then, in commit [96bc59b1](https://gitlab.kitware.com/cmake/cmake/-/blob/96bc59b1ca01be231347404d178445263687dd22/Source/cmLocalGenerator.cxx#L1393-1403), another developer adds new logic and forgets about that safeguard when inserting their code\. Here, static analysis shines and proves its worth: with merge requests, it stands watch, and the code updates automatically are [checked](https://pvs-studio.com/en/docs/manual/0055/)\.

Here's the simplest way to fix the problem:

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

Here's how PVS\-Studio warns about this error: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'pcli' pointer was utilized before it was verified against nullptr\. Check lines: 1445, 1454\. [cmLocalGenerator\.cxx 1445](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmLocalGenerator.cxx#L1445)

The same warnings:

* 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

**Fragment N3**

The PVS\-Studio warning: [V573](https://pvs-studio.com/en/docs/warnings/v573/) Uninitialized variable 'intDir' was used\. The variable was used to initialize itself\. [cmGlobalVisualStudio7Generator\.cxx 626](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmGlobalVisualStudio7Generator.cxx#L626)

```cpp
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);               // <=
    ....
  }
  ....
}
```

Here is an interesting bug puffed right into the master branch in commit [b82a74d9](https://gitlab.kitware.com/cmake/cmake/-/commit/b82a74d91805631b7542ce99896722d7fdc50f5a#a77a481daea2cd01292143b8e40b8decae016977)**\.** The developer loops through the strings in the `configs` container and builds a path to the intermediate directory \(`intDir`\) that will be written to the `*.vcproj` file\.

The problem is that the `intDir` variable is used in its own initialization, which leads to undefined behavior, since the object lifetime will only begin after initialization is complete \([\[1\]](https://timsong-cpp.github.io/cppwp/n4950/basic.scope.pdecl#1), [\[2\]](https://timsong-cpp.github.io/cppwp/n4950/basic.life#1)\)\. We can see that a variable with a similar name and the `Prefix` suffix is created before the loop, and it should probably be used when initializing `intDir`:

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

**Fragment N4**

The PVS\-Studio warning: [V778](https://pvs-studio.com/en/docs/warnings/v778/) Two similar code fragments were found\. Perhaps, this is a typo and 'writeOp' variable should be used instead of 'readOp'\. [cmDebuggerWindowsPipeConnection\.cxx 20](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmDebuggerWindowsPipeConnection.cxx#L20)

```cpp
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);
}
```

Inside the `DuplexPipe_WIN32` constructor, the `readOp` and `writeOp` data members of the [`OVERLAPPED`](https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-overlapped) type are initialized\. In this structure, developers initialize the `Offset`, `OffsetHigh`, and `hEvent` data members\. They start with the `readOp` data member, then move on to `writeOp`\. To initialize it, developers decide to copy the two lines above and change the name\.

Unfortunately, devs have forgotten to change `readOp.OffsetHigh` as well, which could cause some tricky bugs\. It seems that the fix seems obvious; however, we can use another option:

```cpp
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);
}
```

According to the [documentation](https://learn.microsoft.com/en-us/windows/win32/sync/synchronization-and-overlapped-input-and-output):

_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\._

The default member initializers in the class definition will fill everything with nulls in `OVERLAPPED`, and then `readOp.hEvent` and `writeOp.hEvent` will be initialized in the constructor body\.

**Fragment N5**

The PVS\-Studio warning: [V1028](https://pvs-studio.com/en/docs/warnings/v1028/) Possible overflow\. Consider casting operands of the '\_yybytes\_len \+ 2' operator to the 'yy\_size\_t' type, not the result\. [cmExprLexer\.cxx 1851](https://github.com/Kitware/CMake/blob/51e22572ed2294273161ee7a33349e9bd591ba8e/Source/LexerParser/cmExprLexer.cxx#L1851)

```cpp
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;
  ....
}
```

The`yybytes_len` variable has the `int` type and is used to form a new dynamically allocated buffer, `buf`\. Its size is evaluated as a signed expression and converted to unsigned\. Signed overflow, whose behavior is undefined, may occur during addition if `yybytes_len` is in the range `[INT_MAX – 1 .. INT_MAX]`\. The conversion result may also be unexpected\.

Generally, the problem falls under the category of [64\-bit errors](https://pvs-studio.com/en/blog/terms/0002/)\. Unfortunately, to solve this problem, we need to change the function interface: the buffer size parameter should be declared with the `size_t` type, not `int`\. Moreover, this function is already called with an argument of the `size_t` type, which has been [converted](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/LexerParser/cmExprLexer.cxx#L1833) to `int`:

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

Once the function accepts the `size_t` type, explicit type conversions become unnecessary\. Additionally, all loop or index variables used in calculations should also switch from `int` to `size_t`\. In our case, this is the `i` local variable\.

<details>
   <summary>The same warnings:</summary>

* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmListFileLexer\.c 1586
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmListFileLexer\.c 1923
* V1028 Possible overflow\. Consider casting operands of the '\_yybytes\_len \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmListFileLexer\.c 2176
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmFortranLexer\.cxx 1661
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmFortranLexer\.cxx 1990
* V1028 Possible overflow\. Consider casting operands of the '\_yybytes\_len \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmFortranLexer\.cxx 2243
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmExprLexer\.cxx 1272
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmExprLexer\.cxx 1598
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmGccDepfileLexer\.cxx 1275
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmGccDepfileLexer\.cxx 1601
* V1028 Possible overflow\. Consider casting operands of the '\_yybytes\_len \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmGccDepfileLexer\.cxx 1854
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmDependsJavaLexer\.cxx 1860
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmDependsJavaLexer\.cxx 2186
* V1028 Possible overflow\. Consider casting operands of the '\_yybytes\_len \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmDependsJavaLexer\.cxx 2439
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmCTestResourceGroupsLexer\.cxx 1269
* V1028 Possible overflow\. Consider casting operands of the 'b\-\>yy\_buf\_size \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmCTestResourceGroupsLexer\.cxx 1595
* V1028 Possible overflow\. Consider casting operands of the '\_yybytes\_len \+ 2' operator to the 'yy\_size\_t' type, not the result\. cmCTestResourceGroupsLexer\.cxx 1848
* V1028 Possible overflow\. Consider casting operands of the '\* size \* 2' operator to the 'size\_t' type, not the result\. System\.c 31


</details>


**Fragment N6**

```cpp
/**
 * 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;
}
```

Here we have the `SystemTools::AppendStrings` function, which is designed to combine two passed strings in a new buffer and return a pointer to it\.

What can happen if an error occurs when devs attempt to allocate memory? Let's pay attention to the comment on the function: "If an error occurs, the function should return a null pointer\." Will that really happen? Not quite\. The `new` expression throws the [`std::bad_alloc`](https://en.cppreference.com/w/cpp/memory/new/bad_alloc.html) exception when the error occurs, making the following check meaningless\. If no exception is thrown, the `new` expression returns a valid pointer\.

To fix the problem, you need to call the [`nothrow`](https://en.cppreference.com/w/cpp/memory/new/nothrow_t.html) version of the allocation function:

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

This is how the analyzer warns about this problem: [V668](https://pvs-studio.com/en/docs/warnings/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](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/kwsys/SystemTools.cxx#L1724)

The same warning:

* 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

**Fragment N7**

The PVS\-Studio warning: [V557](https://pvs-studio.com/en/docs/warnings/v557/) Array overrun is possible\. The value of 'length \- 1' index could reach 18446744073709551615\. [ProcessWin32\.c 1942](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/kwsys/ProcessWin32.c#L1942)

```cpp
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;
  }
}
```

The utility function is designed to make small adjustments to an error message\. If the message ends with the `.\r\n` sequence, the function will remove characters\. However, before checking that the last character is `\n`, developers don't check the message length\. If it's empty, an unsigned overflow will occur when calculating the index, the final value will be equal to [`SIZE_MAX`](https://en.cppreference.com/w/cpp/types/climits.html), and it leads to buffer overflow and [undefined](https://pvs-studio.com/en/blog/posts/cpp/1241/) behavior\. Interestingly, later in the function, when removing the `.` character, developers correctly check the length\.

Here's the fixed code:

```cpp
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')
  ....
}
```

**Fragment N8**

The PVS\-Studio warning: [V1043](https://pvs-studio.com/en/docs/warnings/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](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmStatePrivate.h#L27)

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

The analyzer has detected a global constant defined in the header file\. When it's included in the translation units, each of them will create its own copy of this constant because it has an [internal linkage](https://pvs-studio.com/en/blog/terms/6506/#ID0AB1BD61C2) due to two reasons:

* It's not `volatile` but `const`\-qualified\.
* It's declared with the `static` specifier\.

A quick glance at the codebase reveals that this header is included in:

* [`cmState.cxx`](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmState.cxx#L24)
* [`cmStateDirectory.cxx`](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmStateDirectory.cxx#L22)
* [`cmStateSnapshot.cxx`](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmStateSnapshot.cxx#L22)

There are two ways to fix this, depending on the standard version used in the project\.

Prior to C\+\+17:

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

// some translation unit

#include "cmStatePrivate.h"

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

Starting with C\+\+17:

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

**Fragment N9**

The PVS\-Studio warning: [V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. This condition was already verified in line 2528\. [cmQtAutoGenInitializer\.cxx 2529](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmQtAutoGenInitializer.h#L2529)

```cpp
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;
        }
      }
    }
  } 
  .... 
}
```

The analyzer has detected a possible redundant check: the `!genVars.ExecutableFeatures.Config[config]` condition is checked twice in a row\. Let's take a closer look, first, at all the necessary types and definitions:

<details>
   <summary>Additional info</summary>

```cpp
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;
  ....
};
```


</details>


The `Config` data member is a container of the `std::unordered_map<std::string, std::shared_ptr<CompilerFeatures>>` type\. When using the `[]` operator, the container will search for an element by the specified key and either return a reference to an existing object or to a new one created by default\. The condition will be evaluated as `true` in two cases:

1. The element exists in the associative container but is null\.
1. The element doesn't exist in the associative container, and a new null smart pointer with the specified key will be created inside it\.

At the same time, no direct modification of the element occurs between two searches\. It does not appear that this code is executed in any way multithreaded, since there are no synchronization mechanisms in the function\. It seems that this part of the code was supposed to check the result of the `GetCompilerFeatures` function: all objects by keys from `ConfigsList` should be present inside the `genVars.ExecutableFeatures` container and be non\-null\. If any of these conditions are violated, the function should log an error and return `false`\.

Based on this idea, let's simplify the code:

```cpp
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;
  }
}
```

The same warning:

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

**Fragment N10**

The PVS\-Studio warning: [V728](https://pvs-studio.com/en/docs/warnings/v728/) An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions 'arguments\.ErrorVariable\.empty\(\)' and '\!arguments\.ErrorVariable\.empty\(\)'\.  [cmExecuteProcessCommand\.cxx 257](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmExecuteProcessCommand.cxx#L257)

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

The analyzer issues a warning that the logical expression contains redundant checks\. The `arguments.ErrorVariable.empty()` expression is used twice during evaluations, and the second time it's inverted\. If this expression is evaluated as `false` as the left operand of the `||` operator, then he second evaluation on the right is guaranteed to be `true`\.

Let's simplify the logical expression:

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

<details>
   <summary>Proof of logical expression equivalence</summary>

If the correction doesn't seem obvious at first glance, let's prove that the two logical expressions are equivalent using a truth table:

* the `A` expression is `arguments.ErrorVariable.empty()`; 
* the `B` expression is `arguments.OutputVariable != arguments.ErrorVariable`\.

![1277_CMake_v4_1_0/image2.png](https://import.viva64.com/docx/blog/1277_CMake_v4_1_0/image2.png)



See that? The last two columns are identical, which means that the two resulting expressions are equivalent for all possible combinations of sub\-expression values\. Thus, the simplified expression keeps the same logic as the original; however, the simplified version is easier to understand\.


</details>


The same warnings:

* 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

**Fragment N11**

The PVS\-Studio warning: [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '\!this\-\>MakefileVariableSize' to the left and to the right of the '&&' operator\. [cmLocalUnixMakefileGenerator3\.cxx 1292](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmLocalUnixMakefileGenerator3.cxx#L1292)

```cpp
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;
  }
}
```

In the `cmLocalUnixMakefileGenerator3::CreateMakeVariale` function, the `!this->MakefileVariableSize` condition is repeated unnecessarily before each check for `.`, `+`, and `-` in the `unmodified` string\. This repetition bloats the code and makes it harder to read\. Since `!this->MakefileVariableSize` is common to all three checks, we can move it outside the parentheses and combine all checks into a single logical expression:

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

Well, we've removed the same subexpression, but the condition checks the existence of three find characters in the string\. We can go a step further and perform micro\-optimizations\. Let's replace the call to the `find` member function with `find_first_of`, passing all three characters in a string literal:

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

The same warning:

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

**Fragment N12**

The PVS\-Studio warning: [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the subsequent code fragment\. [cmComputeLinkInformation\.cxx 1748](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmComputeLinkInformation.cxx#L1748)

```cpp
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;
}
```

The analyzer hints that there is definitely something wrong with the `CheckImplicitDirItem` function:

* the `then` branch of the last `if` statement duplicates the code below \(`return false;`\);
* every branch of the function execution ends with `return false`;
* when called from [`AddFullItem`](https://gitlab.kitware.com/cmake/cmake/-/blob/v4.1.0/Source/cmComputeLinkInformation.cxx#L1685), it'll never trigger an early return;
* the full function body can be replaced with `return false;`, as this won't change the observed program behavior\.

Note that the function is written using the [early return pattern](https://szymonkrajewski.pl/why-should-you-return-early/) that helps reduce code nesting: the most _positive_ result is placed at the end of the function, and the rest of the code, in case of divergence from the function's purpose, should terminate the function as early as possible\.

In our example, we can assume that the most _positive_ result of the function is that an object of the `LinkEntry` type has passed all the necessary checks with a return value of `true`\.

Here's the option on how to fix the code:

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

return true;
```

The same warnings:

* 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

## Outro

At this point, our journey with the all\-mighty CMake draws to a close\. We've faced various challenges from copy\-paste to redundant checks\. Yet, despite these trials, CMake stands as a powerful and trustworthy companion\. Mistakes happen to even the bravest of heroes, but they don't define their worth\. CMake is a project that is actively developing, and its bugs are swiftly fixed\. Many of the previously discovered bugs we've encountered have already been slain, confirming that CMake remains a reliable and indispensable tool in the realm of development\.

If your own projects in C, C\+\+, C\#, or Java need a keen\-eyed sentry, our [analyzer](https://pvs-studio.com/en/pvs-studio/download/) is ready to ride alongside you and checks your code\!