# Checking the Source Code of Nana Library with PVS\-Studio

With the appearance of new C\+\+ standards, C\+\+ developers started to move to the new programming style, known as Modern C\+\+, and projects that make use of the new style started to appear\. Static code analyzers have to keep up to date  to be able to detect errors in modern C\+\+ code, which holds true for PVS\-Studio as well\. With Nana project as a test sample, we'll try to find out if PVS\-Studio analyzer can cope with modern C\+\+\.



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

## Introduction

To begin with, I'd like to say a few words about the project\. Nana is a cross\-platform C\+\+11 library for creating graphical user interfaces\. The library is small \- 74 KLOC\. It supports Windows and Linux \(X11\) platforms and provides experimental macOS support\. Nana is an open\-source software product distributed under the [Boost Software License](https://www.boost.org/users/license.html)\. We took version 1\.3\.0 for the check; its source code can be downloaded here: [https://sourceforge\.net/projects/nanapro/files/latest/download](https://sourceforge.net/projects/nanapro/files/latest/download)\.

## Typos in conditions

Typos in conditional statements are rather common, and Nana has a few of those too, for example:

[V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'fgcolor\.invisible\(\)' to the left and to the right of the '&&' operator\. text\_editor\.cpp 1316

```cpp
void text_editor::set_highlight(
  const std::string& name,
  const ::nana::color& fgcolor,
  const ::nana::color& bgcolor)
{
  if (fgcolor.invisible() && fgcolor.invisible())  // <=
  {
    keywords_->schemes.erase(name);
    return;
  }
  ....
}
```

The analyzer detected two identical conditional expressions\. This code was very likely written using [Copy\-Paste](https://pvs-studio.com/en/blog/terms/0068/): the programmer copied the _fgcolor\.invisible\(\)_ expression but forgot to change the _fgcolor_ variable's name\. Judging by the function's parameters, the programmer wanted to use argument _bgcolor _instead of _fgcolor_ in the second subexpression\. In that case, the fixed code should look like this:

```cpp
void text_editor::set_highlight(
  const std::string& name,
  const ::nana::color& fgcolor,
  const ::nana::color& bgcolor)
{
  if (fgcolor.invisible() && bgcolor.invisible())
  {
    keywords_->schemes.erase(name);
    return;
  }
  ....
}
```

There's almost no way we can do without Copy\-Paste when writing code, so you just have to be more careful when inspecting copy\-pasted and amended code\. If you have still made a mistake because of copy\-paste, static analysis could help save time when tracking it down\.

## Using a null pointer \(not a bug here, but code like that is always worth checking\)

The analyzer detected a code fragment where a null pointer is used\.

[V522](https://pvs-studio.com/en/docs/warnings/v522/) Dereferencing of the null pointer 'debug' might take place\. text\_token\_stream\.hpp 669

This is what it looks like:

```cpp
void parse(....)
{
  ....
  switch(tk)
  {
    ....
    default:
      int * debug = 0;  //for debug.
      *debug = 0;
  }
  ....
}
```

As the comment suggests, this trick with the _debug_ pointer was done for debugging purposes, but I still felt I should cite this fragment as an example\. You must be very careful with things like that, as program logic might change later and you'd be unpleasantly surprised\. Anyway, this code should be re\-checked just in case\.

## Incorrect use of a smart pointer

We have finally got to an issue related to the C\+\+11 standard\. The analyzer detected a situation where using a smart pointer might cause undefined behavior, in particular damaging the heap or a program crash\. The reason is that the programmer uses different methods to allocate and free memory\.

[V554](https://pvs-studio.com/en/docs/warnings/v554/) Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. text\_editor\.cpp 3137

```cpp
void text_editor::_m_draw_string(....) const
{
  ....
  for (auto & ent : reordered)
  {
    ....
    std::size_t len = ent.end - ent.begin;
    ....
    if (....)
    {
      ....
    }
    else if (pos <= a.x && a.x < str_end)
    {
      ....
      std::unique_ptr<unsigned> pxbuf_ptr(new unsigned[len]);  // <=
    }
  }
}
```

The [_unique\_ptr \[RU\]_](https://ru.cppreference.com/w/cpp/memory/unique_ptr) class is used to manage the memory block allocated for the array\. When freeing that block, it uses the _delete _operator by default, which results in undefined behavior\. To fix this error, we need to use partial specialization of _unique\_ptr_ for the array\. In that case, memory will be freed by calling the _delete\[\]_ operator\. This is what the fixed code should look like:

```cpp
std::unique_ptr<unsigned[]> pxbuf_ptr(new unsigned[len]);
```

## Redundant comparison

Sometimes conditional statements use redundant checks, which may contain potential errors\. The analyzer detected a redundant comparison in the code and issued the following warning:

[V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. window\_manager\.cpp 467

```cpp
void window_manager::destroy_handle(core_window_t* wd)
{
  ....
  if((wd->other.category == category::root_tag::value) ||
     (wd->other.category != category::frame_tag::value))  // <=
  {
   impl_->misc_register.erase(wd->root);
   impl_->wd_register.remove(wd);
  }
}
```

Here's a simple example to explain the point:

```cpp
if (a == 1 || a != 5)
```

The condition will execute if _a \!\= 5_\. The first part of the expression makes no sense\. If you examine the code closely, you will come to one of the following conclusions: either the expression should be simplified by removing the first part \- the code would then look like this:

```cpp
if (a != 5)
```

or there is an error in the expression, in which case it should be fixed like this:

```cpp
if (a == 1 || not_a_var != 5)
```

In our example, the first situation is more probable, so it should be simplified in the following way:

```cpp
void window_manager::destroy_handle(core_window_t* wd)
{
  ....
  if(wd->other.category != category::frame_tag::value)
  {
   impl_->misc_register.erase(wd->root);
   impl_->wd_register.remove(wd);
  }
}
```

## On dangerous uses of pointers one more time

You must be especially careful when using pointers in C/C\+\+\. If you are not sure there are any data found at the address pointed to by a pointer, make sure you verify it against null\. Accessing memory pointed by a null pointer results in undefined behavior or a program crash\.

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'owner' pointer was utilized before it was verified against nullptr\. Check lines: 299, 315\. window\_manager\.cpp 299

```cpp
window_manager::core_window_t*
window_manager::create_root(core_window_t* owner,    )
{
  ....
  if (nested)
  {
    wd->owner = nullptr;
    wd->parent = owner;
    wd->index = static_cast<unsigned>(owner->children.size());
    owner->children.push_back(wd);  // <=
  }
  ....
  if (owner 
      && owner->other.category == category::frame_tag::value)  // <=
    insert_frame(owner, wd);
  ....
}
```

V595 is perhaps the most common warning across all the projects we check\. Here's another similar issue found in this one:

V595 The 'wd' pointer was utilized before it was verified against nullptr\. Check lines: 1066, 1083\. window\_manager\.cpp 1066

## Using the SuspendThread\(\) function

The analyzer detected a code fragment where the [_SuspendThread\(\)_](https://msdn.microsoft.com/en-us/library/windows/desktop/ms686345(v=vs.85).aspx) function is used:

[V720](https://pvs-studio.com/en/docs/warnings/v720/) It is advised to utilize the 'SuspendThread' function only when developing a debugger \(see documentation for details\)\. pool\.cpp 225

```cpp
void _m_suspend(pool_throbj* pto)
{
  pto->thr_state = state::idle;
#if defined(NANA_WINDOWS)
  ::SuspendThread(pto->handle);  // <=
#elif defined(NANA_POSIX)
  std::unique_lock<std::mutex> lock(pto->wait_mutex);
  pto->suspended = true;
  pto->wait_cond.wait(lock);
  pto->suspended = false;
#endif
}
```

A call to this function is not an error in itself; however, developers often use it inappropriately, which might result in unexpected behavior\. The _SuspendThread\(\)_ function is meant to help developers create debuggers and other similar utilities\. If you use it for synching purposes, you are very likely to get a bug\.

For more information on misuses of the _SuspendThread\(\)_ function, see the following articles:

* [Why you should never suspend a thread](https://devblogs.microsoft.com/oldnewthing/2003/12/09)\.
* [The SuspendThread function suspends a thread, but it does so asynchronously](https://devblogs.microsoft.com/oldnewthing/2015/02/05)

## Conclusion

Nana is a small project and there aren't many errors in it\. However, some of the fragments do need to be checked\. Among the errors found, there is one related to the use of the C\+\+11 standard\. One error is, of course, not enough to estimate PVS\-Studio's capabilities for analyzing C\+\+11 projects, so we would be glad to receive any suggestions from you\. If you know any projects written in modern C\+\+, please let us know, and we'll try to check them\. Use the [feedback form](https://pvs-studio.com/en/about-feedback/) to contact us\.

To sum up, I'd like to warn you against limiting yourself to tests and code review when writing program code\. Using static analysis helps write better code and save time when searching for bugs\. So, you are welcome to try [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) on your projects written in C, C\+\+, or C\#\.