Analysis of the Ultimate Toolbox project

While testing the general analyzer included into PVS-Studio 4.00, we checked several open-source projects from the CodeProject site. One of those was Ultimate ToolBox.

We found some errors in the code of the Ultimate Toolbox project and would like to describe them further in this article. For each case, we will give the diagnostic message generated by the analyzer, corresponding file and line number. We will also give the code fragment containing a particular error and a brief error description. To study the samples thoroughly, you may visit the resources by the links given in the text.

1. Condition error

V501 There are identical sub-expressions to the left and to the right of the '&&' operator. UT ox3dtabview.cpp 230

void COX3DTabViewContainer::OnNcPaint() 
{
  ...
  if(rectClient.top<rectClient.bottom &&
     rectClient.top<rectClient.bottom)
  {
    dc.ExcludeClipRect(rectClient);
  }
  ...
}

The V501 warning points to a condition error. It is most likely that there must be a condition comparing left and right after the '&&' operator.

A similar error can also be found here:

V501 There are identical sub-expressions to the left and to the right of the '&&' operator. UT oxtabclientwnd.cpp 184

2. Condition which is always true.

V547 Expression 'lpDrawItemStruct -> itemID >= 0' is always true. Unsigned type value is always >= 0. UT oxautolistbox.cpp 56

void COXAutoListBox::DrawItem(...) 
{
  ...
  if (lpDrawItemStruct->itemID>=0)
  {
    ...
  }
  ...
}

The "lpDrawItemStruct->itemID>=0" condition always holds because the itemID member has the UINT type. Such errors are described in detail in documentation (V547). The code must have looked this way:

if (lpDrawItemStruct->itemID != (UINT)(-1))
{
  ...
}

3. Condition which is always false.

V547 Expression 'lpms -> itemID < 0' is always false. Unsigned type value is never < 0. UT oxcoolcombobox.cpp 476

void COXCoolComboBox::MeasureItem(...)
{
  if(lpms->itemID<0)
    lpms->itemHeight=m_nDefaultFontHeight+1; 
  else
    lpms->itemHeight=
      m_nDefaultFontHeightSansLeading+1; 
}

The V547 warning tells us that the code "lpms->itemHeight=m_nDefaultFontHeight+1;" will be always executed. Like in the previous case, it is caused by the fact that the itemID member has the unsigned type UINT.

4. Inefficient code

V801 Decreased performance. It is better to redefine the first function argument as a reference. Consider replacing 'const .. mi' with 'const .. &mi'. UT oxdllmanager.h 123

BOOL operator==(const _tagMODULEINFO mi) const 
{
 return ((hModule==mi.hModule)&
   (sModuleFileName.CompareNoCase(mi.sModuleFileName)==0));
}

This code does not contain an error, but we may make it more efficient. There is no need in creating a copy of the _tagMODULEINFO structure each time the '==' operator is called. The V801 message tells us that we may replace "const _tagMODULEINFO mi" with "const _tagMODULEINFO &mi".

5. Condition error

V501 There are identical sub-expressions to the left and to the right of the '==' operator: dwDockStyle == dwDockStyle UT oxframewnddock.cpp 190

void COXFrameWndSizeDock::TileDockedBars(
  DWORD dwDockStyle)
{
  ...
  if (pDock != NULL &&
      (pDock->m_dwStyle &&
       dwDockStyle == dwDockStyle))
  ...
}

It is most likely that the programmer intended to write some other expression instead of the "dwDockStyle == dwDockStyle" expression.

6. Handling 'char' as 'unsigned char'

Two warnings at once were given for one line:

V547 Expression 'chNewChar >= 128' is always false. The value range of signed char type: [-128, 127]. UT oxmaskededit.cpp 81

V547 Expression 'chNewChar <= 255' is always true. The value range of signed char type: [-128, 127]. UT oxmaskededit.cpp 81

BOOL CMaskData::IsValidInput(TCHAR chNewChar)
{
   ...
  if((chNewChar >= 128) && (chNewChar <= 255))
    bIsValidInput=TRUE ;
  ...
}

This condition is meaningless since the chNewChar variable's value range is [-128..127]. It means that the condition will never hold.

7. Logic error

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. UT oxprocess.h 583

inline COXProcessIterator& operator+(int nOffset)
{
  if(nOffset>0)
    Next(nOffset);
  else if(nOffset>0)
    Prev(-nOffset);
  return *this; 
}

The V517 warning points to an error in program's logic. The "Prev(-nOffset);" branch will never be executed. The correct code must look as follows:

inline COXProcessIterator& operator+(int nOffset)
{
  if(nOffset>0)
    Next(nOffset);
  else if(nOffset<0)
    Prev(-nOffset);
  return *this; 
}

There are similar errors in other program's fragments:

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. UT oxprocess.h 596

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. UT oxprocess.h 610

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. UT oxprocess.h 624

8. Condition which is always false.

V547 Expression 'm_nCurrentIndex - nOffset < 0' is always false. Unsigned type value is never < 0. UT oxprocess.cpp 594

int m_nCurrentIndex;
...
BOOL COXProcessIterator::Prev(UINT nOffset)
{
  ...
  if(m_nCurrentIndex-nOffset<0)
    return FALSE;
  ...
}

Since the "m_nCurrentIndex-nOffset" expression has the unsigned type, it will never be below 0.

9. Error ASSERT()

V501 There are identical sub-expressions to the left and to the right of the '&&' operator. UT oxscrollwnd.cpp 645

void COXScrollWnd::OnPrepareDC(...)
{
  ...
  ASSERT(m_totalDev.cx>=0 && m_totalDev.cx>=0);
  ...
}

There must be this code:

ASSERT(m_totalDev.cx>=0 && m_totalDev.cy>=0);

There is also a similar error here:

V501 There are identical sub-expressions to the left and to the right of the '&&' operator. UT oxzoomvw.cpp 179