PVS-Studio vs CodeLite: a battle for the perfect code

• About CodeLite
• Analysis results
• Conclusion

How to improve the quality and reliability of a codebase? One of the answers to this question is to use static analysis. In this article, we are going to check how this methodology can improve the codebase quality. We will use the CodeLite project as an example.

About CodeLite

CodeLite is a free integrated development environment (IDE) for various programming languages including C, C++, PHP, and JavaScript. It is designed to simplify the software development process; it offers many features and tools.

Key features of CodeLite:

• Code editor: CodeLite provides a convenient and powerful code editor with syntax highlighting, autocomplete, fast navigation capabilities, and other useful tools.
• Debugger: the IDE includes a debugger that allows you to monitor program execution, set breakpoints, and analyze the state of variables.
• Build system: CodeLite supports several build systems, including Make, CMake, and others. It makes compilation and project building easier.
• Git integration: the IDE has built-in support for the Git version control system. It allows developers to easily work with Git repositories directly from the development environment.
• Additional extensions support: CodeLite supports plugins that allow users to extend the functionality of the IDE according to their own needs.
• Portability: CodeLite is available for Windows, macOS, and Linux operating systems, making it a great choice for cross-platform developers.

CodeLite is an open-source project. It allows programmers to take part in its development and to help improve the IDE. It is continually updated and enhanced to provide powerful tools for programmers.

A lot of time has passed since we published the last article about CodeLite's check in the PVS-Studio blog. Can't wait to find out what kind of new bugs we are going to find.

Analysis results

The analyzer issued a lot of warnings while checking CodeLite 17.0.0. As a result, this article will only cover those code fragments that caught my attention while I was going over some of the warnings. If you are the project authors and you find this article interesting, you may check the project yourself. This way you can have a closer look at the list of warnings. You may use a trial version. If you enjoy our tool and would like to use it on a regular basis, there is a free license available for open-source projects.

Fragment N1

The analyzer warning: V554 Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. clSocketBase.cpp:282

int clSocketBase::ReadMessage(wxString& message, int timeout)
{
  ....
  size_t message_len(0);
  ....
  message_len = ::atoi(....);
  ....
  std::unique_ptr<char> pBuff(new char[message_len]);
  ....
}

The analyzer has detected an issue: the use of smart pointers can cause undefined behavior. In the code, the std::unique_ptr class template is instantiated with the char type. Therefore, a specialization, whose destructor uses the operator delete to release an object, is chosen. However, an array, allocated using the operator new[], is passed to the smart pointer. Here you can read why, in C++, arrays should be deleted using the operator delete[], and why undefined behavior occurs.

To fix the error, instantiate the std::unique_ptr class template with the char[] type:

std::unique_ptr<char[]> pBuff { new char[message_len] };

Fragment N2

The analyzer warning: V762 It is possible a virtual function was overridden incorrectly. See third argument of function 'Update' in derived class 'clProgressDlg' and base class 'wxGenericProgressDialog'. progress_dialog.h:47, progdlgg.h:44

The analyzer has detected an incorrect virtual function overriding. Here's what the function looks like in the base class:

class WXDLLIMPEXP_CORE wxGenericProgressDialog : public wxDialog
{
public:
....

    virtual bool Update(int value,
                        const wxString& newmsg = wxEmptyString,
                        bool *skip = NULL);

....
};

And here's what the function looks like in the derived class:

class clProgressDlg : public wxProgressDialog
{
public:
    ....
    bool Update(int value, const wxString& msg);
    ....
};

Since the first two function parameters are the same, we can conclude that the developers wanted to override the virtual function. However, the default parameters are also a part of the function signature. Therefore, the clProgressDlg::Update function does not actually override the wxGenericProgressDialog::Update virtual function, but hides it.

The correct way to declare a virtual function is as follows:

class clProgressDlg : public wxProgressDialog
{
public:
    ....
    bool Update(int value, const wxString& msg, bool *skip);
    ....
};

Since C++11, it is possible (and even necessary) to use the override specifier to avoid such errors:

class clProgressDlg : public wxProgressDialog
{
public:
    ....
    bool Update(int value, const wxString& msg, bool *skip) override;
    ....
};

Now the compiler throws an error if the virtual function does not override anything in the base class.

Let's take a look at similar warnings:

Fragment N3

The analyzer warning: V595 The 'dbgr' pointer was utilized before it was verified against nullptr. Check lines: 349, 351. simpletable.cpp:349, simpletable.cpp:351

void WatchesTable::OnCreateVariableObject(....)
{
  ....
  if (dbgr->GetDebuggerInformation().defaultHexDisplay == true)
    dbgr->SetVariableObbjectDisplayFormat(DoGetGdbId(item),
                                        DBG_DF_HEXADECIMAL);

  if (dbgr)
    DoRefreshItem(dbgr, item, true);
  ....
}

The analyzer has detected the following error: a pointer is dereferenced first, and only then it is checked for NULL.

Let's take a look at similar warnings:

Fragment N4

The analyzer warning: V766 An item with the same key ''.'' has already been added. wxCodeCompletionBoxManager.cpp:19

std::unordered_set<wxChar> delimiters =
  { ':', '@', '.', '!', ' ', '\t', '.', '\\', 
    '+', '*', '-', '<', '>', '[', ']', '(', 
    ')', '{', '}',  '=', '%', '#', '^', '&', 
    '\'', '"', '/', '|',  ',', '~', ';', '`' };

Can you see what's wrong here? Because of all the single quotation marks, the eye may not see that the '.' character has been added again. It is possible that they forgot to add some other character here, or it's just a random duplicate that can be removed.

Another similar fragment:

• V766 An item with the same key '"MSYS2/GCC"' has already been added. compiler.cpp:621, compiler.cpp:620

Fragment N5

The analyzer warning: V501 There are identical sub-expressions 'result.second.empty()' to the left and to the right of the '||' operator. RemotyNewWorkspaceDlg.cpp:19

void RemotyNewWorkspaceDlg::OnBrowse(wxCommandEvent& event)
{
  auto result = ::clRemoteFileSelector(_("Seelct a folder"));
  if (result.second.empty() || result.second.empty())
  {
    return;
  }
  ....
}

Developers made a typo: there are identical subexpressions to the left and right of the operator ||. In addition to the second data member, they should have checked the first data member as well. Sometimes the analyzer's warning indirectly reveals other anomalies in the code. For example, an incorrect string literal is passed to the ::clRemoteFileSelector function :)

The fixed code:

auto result = ::clRemoteFileSelector(_("Select a folder"));
if (result.first.empty() || result.second.empty())
{
  return;
}

The analyzer has detected two more similar fragments:

• V501 There are identical sub-expressions '!sshSettings.IsRemoteUploadEnabled()' to the left and to the right of the '||' operator. PhpSFTPHandler.cpp:104
• V501 There are identical sub-expressions 'output.Contains("username for")' to the left and to the right of the '||' operator. git.cpp:1715

Fragment N6

The analyzer warning: V1043 A global object variable 'GMON_FILENAME_OUT' is declared in the header. Multiple copies of it will be created in all translation units that include this header file. static.h:41

// static.h

#include <wx/string.h>

const wxString GMON_FILENAME_OUT = "gmon.out";
....

The analyzer has detected a constant instance declaration of the wxString class in the header file. According to the C++ standard, constants declared in a namespace have internal linkage. Including such a file using #include will create multiple copies of the object.

Depending on the C++ standard you use, there are two ways to avoid this behavior. Starting with C++17, you can declare a variable with the inline specifier. This feature is very useful when you write header-only libraries. In versions prior to C++17, you need to declare a variable with the extern specifier in a header file and put its definition in a compiled file:

// Since C++17

// static.h

#include <wx/string.h>

inline const wxString GMON_FILENAME_OUT = "gmon.out";

// -----------------------------------------------------

// Until C++17

// static.h

#include <wx/string.h>

extern const wxString GMON_FILENAME_OUT;

// static.cpp

#include "static.h"

const wxString GMON_FILENAME_OUT = "gmon.out";

Let's take a look at similar warnings:

Fragment N7

The analyzer warning: V773 Visibility scope of the 'imageList' pointer was exited without releasing the memory. A memory leak is possible. acceltabledlg.cpp:61, acceltabledlg.cpp:47

AccelTableDlg::AccelTableDlg(wxWindow* parent)
  : AccelTableBaseDlg(parent)
{
  wxImageList* imageList = new wxImageList(16, 16); // <=
  imageList->Add(PluginManager::Get()->
                                GetStdIcons()->
                                LoadBitmap("list-control/16/sort"));
  imageList->Add(PluginManager::Get()->
                                GetStdIcons()->
                                LoadBitmap("list-control/16/sort"));

  clKeyboardManager::Get()->GetAllAccelerators(m_accelMap);
  PopulateTable("");

  CentreOnParent();

  m_textCtrlFilter->SetFocus();

  SetName("AccelTableDlg");
  WindowAttrManager::Load(this);
}

The analyzer has detected a possible memory leak. It looks like the developers forgot to pass the imageList variable somewhere.

Here are more fragments that look suspicious:

• V773 Visibility scope of the 'pDump' pointer was exited without releasing the memory. A memory leak is possible. ErdCommitWizard.cpp:273, ErdCommitWizard.cpp:219
• V773 The function was exited without releasing the 'argv' pointer. A memory leak is possible. unixprocess_impl.cpp:288, unixprocess_impl.cpp:286
• V773 The function was exited without releasing the 'child' pointer. A memory leak is possible. compilersfoundmodel.cpp:135, compilersfoundmodel.cpp:127
• V773 The function was exited without releasing the 'child' pointer. A memory leak is possible. xdebuglocalsviewmodel.cpp:135, xdebuglocalsviewmodel.cpp:127

Fragment N8

The analyzer warning: V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 372, 375. clTreeCtrlModel.cpp:375, clTreeCtrlModel.cpp:372

bool clTreeCtrlModel::GetRange(....) const
{
  items.clear();

  if (from == nullptr || to == nullptr)
  {
    return false;
  }

  if (from == nullptr)
  {
    items.push_back(to);
    return true;
  }

  if (to == nullptr)
  {
    items.push_back(from);
    return true;
  }
  ....
}

Take a look at the first if. Control flow goes to the next if only if from != nullptr && to != nullptr. It means that the control flow will not go to any of the following if's.

In fact, the first check should have looked like this:

if (from == nullptr && to == nullptr)
{
  return false;
}

The analyzer has detected a similar fragment:

• V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 1998, 2000. ShapeCanvas.cpp:2000, ShapeCanvas.cpp:1998

Fragment N9

The analyzer warning: V668 There is no sense in testing the 'pDump' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. ErdCommitWizard.cpp:220

void BackupPage::OnBtnBackupClick(wxCommandEvent& event)
{
  ....
  DumpClass* pDump = new DumpClass(....);
  if (pDump) dumpResult = pDump->DumpData();
  ....
}

In the code above, the value of the pointer returned by the operator new is compared to null. This operation is pointless. When the check for null happens, the pointer is always valid.

If memory cannot be allocated, the std::bad_alloc exception is thrown. If exceptions are disabled, std::abort is called. In any case, the value of the pDump pointer is always non-null.

Let's take a look at similar warnings:

Fragment N10

The analyzer warning: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 483, 484. SqlCommandPanel.cpp:484, SqlCommandPanel.cpp:483

wxArrayString SQLCommandPanel::ParseSql() const
{
  ....
  int startPos = 0;
  int stopPos = 0;
  ....
  startPos = stopPos;
  stopPos = startPos;
  ....  
}

The analyzer has detected a suspicious sequence of variable assignments. Perhaps the developers intended to swap the variables. Instead, they set them to the same value.

Fragment N11

The analyzer warning: V590 Consider inspecting the 'where != std::string::npos && where == 0' expression. The expression is excessive or contains a misprint. dbgcmd.cpp:60

void wxGDB_STRIP_QUOATES(wxString& currentToken)
{
  size_t where = currentToken.find(wxT("\""));

  if (where != std::string::npos && where == 0) {
     currentToken.erase(0, 1);
  }
  ....
}

If the string begins with the double quotation mark, the code removes it. According to the standard, std::string::npos always equals to the maximum value represented by the size_t type. That is, std::string::npos will never be 0.

In fact, there is a way to enhance the code:

if (!currentToken.empty() && currentToken[0] == wxT('"')) 
{
   currentToken.erase(0, 1);
}

Here's another similar warning:

• V590 Consider inspecting the 'where != std::string::npos && where == 0' expression. The expression is excessive or contains a misprint. dbgcmd.cpp:70

Fragment N12

The analyzer warning: V523 The 'then' statement is equivalent to the 'else' statement. mainbook.cpp:450, mainbook.cpp:442

void MainBook::GetAllEditors(clEditor::Vec_t& editors, size_t flags)
{
  ....
  if (!(flags & kGetAll_DetachedOnly))
  {
    if (!(flags & kGetAll_RetainOrder))
    {
      // Most of the time we don't care about
      // the order the tabs are stored in
      for (size_t i = 0; i < m_book->GetPageCount(); i++)
      {
        clEditor* editor = dynamic_cast<clEditor*>(m_book->GetPage(i));
        if (editor)
        {
          editors.push_back(editor);
        }
      }
    }
    else
    {
      for (size_t i = 0; i < m_book->GetPageCount(); i++)
      {
        clEditor* editor = dynamic_cast<clEditor*>(m_book->GetPage(i));
        if (editor)
        {
          editors.push_back(editor);
        }
      }
    }
  }
  ....
}

The analyzer has detected a fragment where then and else branches of the if statement are completely identical. Here are eight more similar warnings:

• V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp:289, art_metro.cpp:287
• V523 The 'then' statement is equivalent to the 'else' statement. clStatusBar.cpp:151, clStatusBar.cpp:147
• V523 The 'then' statement is equivalent to the 'else' statement. php_workspace_view.cpp:1001, php_workspace_view.cpp:999
• V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp:334, art_metro.cpp:332
• V523 The 'then' statement is equivalent to the 'else' statement. symboldatabase.cpp:1299, symboldatabase.cpp:1297
• V523 The 'then' statement is equivalent to the 'else' statement. tokenize.cpp:10387, tokenize.cpp:10381
• V523 The 'then' statement is equivalent to the 'else' statement. parser.hpp:155, parser.hpp:151
• V523 The 'then' statement is equivalent to the 'else' statement. sizer_flags_list_view.cpp:125, sizer_flags_list_view.cpp:122

Fragment N13

The analyzer warning: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 203, 213. new_quick_watch_dlg.cpp:203, new_quick_watch_dlg.cpp:213

void DisplayVariableDlg::UpdateValue(....)
{
  ....
  wxTreeItemId item = iter->second;

  if (item.IsOk())
  {
    ....
  }
  else if (item.IsOk())
  {
    ....
  }
  ....
}

In the example above, the same item.IsOk() condition is passed to if and else if. We're dealing with a logical error here.

Here are two similar warnings:

• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 53, 55. symbol_tree.cpp:53, symbol_tree.cpp:55
• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 82, 84. symbol_tree.cpp:82, symbol_tree.cpp:84

Fragment N14

The analyzer warning: V614 Uninitialized buffer 'buf' used. Consider checking the first actual argument of the 'Write' function. wxSerialize.cpp:1039

bool wxSerialize::WriteDouble(wxFloat64 value)
{
  if (CanStore())
  {
    SaveChar(wxSERIALIZE_HDR_DOUBLE);

    wxInt8 buf[10];
    m_odstr.Write(buf, 10);
  }

  return IsOk();
}

The analyzer has detected the use of the buf uninitialized variable. It may cause unpredictable results.

Let's take a look at similar warnings:

• V614 Potentially uninitialized pointer 'm_item' used. wxc_aui_tool_stickiness.cpp:8
• V614 Potentially uninitialized variable 'err' used. cppcheck.cpp:175
• V614 The 'p' smart pointer is utilized immediately after being declared or reset. It is suspicious that no value was assigned to it. connection_impl.hpp:2200

Fragment N15

The analyzer warning: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!false' and 'false'. clDebuggerBreakpoint.cpp:26

clDebuggerBreakpoint::clDebuggerBreakpoint(const clDebuggerBreakpoint& BI)
{
  ....
  if (!is_windows || (is_windows && !file.Contains("/"))) 
  {
    ....
  }
  ....
}

The analyzer has detected code that can be simplified. Opposite expressions are to the left and right of the operator ||. The code is redundant — we can simplify it by reducing the number of checks:

if (!is_windows || !file.Contains("/"))
{
  ....
}

The analyzer has detected 17 similar fragments, take a look at some of them:

• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!matcher' and 'matcher'. ssh_account_info.cpp:108
• V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. assignedfilesmodel.cpp:310
• V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'data->m_wxcWidget->IsSizer()' and '!data->m_wxcWidget->IsSizer()'. wxguicraft_main_view.cpp:530

Conclusion

In conclusion, I would like to point out that this article is my first attempt to analyze code using PVS-Studio. I have gained valuable experience in the field of static code analysis. The bugs found in the CodeLite project showed me the importance of such analysis. Based on the analysis results, the project authors can further develop and enhance the code to provide users with an IDE of higher quality.

Following the tradition, let me end the article by offering you to try out the PVS-Studio analyzer. We also provide a free license for open-source projects.