V547. Expression is always true/false

12.03.2012

The analyzer detected a potential error: a condition is always true or false. Such conditions do not always signal an error but still you must review such code fragments.

Consider a code sample:

LRESULT CALLBACK GridProc(HWND hWnd,
  UINT message, WPARAM wParam, LPARAM lParam)
{
  ...
  if (wParam<0)
  {
    BGHS[SelfIndex].rows = 0;
  }
  else
  {
    BGHS[SelfIndex].rows = MAX_ROWS;
  }
  ...
}

The "BGHS[SelfIndex].rows = 0;" branch here will never be executed because the wParam variable has an unsigned type WPARAM which is defined as "typedef UINT_PTR WPARAM".

Either this code contains a logical error or we may reduce it to one line: "BGHS[SelfIndex].rows = MAX_ROWS;".

Now let's examine a code sample which is correct yet potentially dangerous and contains a meaningless comparison:

unsigned int a = _ttoi(LPCTSTR(str1));
if((0 > a) || (a > 255))
{
  return(FALSE);
}

The programmer wanted to implement the following algorithm.

1) Convert a string into a number.

2) If the number lies outside the range [0..255], return FALSE.

The error here is in using the 'unsigned' type. If the _ttoi function returns a negative value, it will turn into a large positive value. For instance, value "-3" will become 4294967293. The comparison '0 > a' will always return true. The program works correctly only because the range of values [0..255] is checked by the 'a > 255' condition.

The analyzer will generate the following warning for this code fragment: "V547 Expression '0 > a' is always false. Unsigned type value is never < 0."

We should correct this fragment this way:

int a = _ttoi(LPCTSTR(str1));
if((0 > a) || (a > 255))
{
  return(FALSE);
}

Let's consider one special case. The analyzer generates the warning:

V547 Expression 's == "Abcd"' is always false. To compare strings you should use strcmp() function.

for this code:

const char *s = "Abcd";
void Test()
{
  if (s == "Abcd")
    cout << "TRUE" << endl;
  else
    cout << "FALSE" << endl; 
}

But it is not quite true. This code still can print "TRUE" when the 's' variable and Test() function are defined in one module. The compiler does not produce a lot of identical constant strings but uses one string. As a result, the code sometimes seems quite operable. However, you must understand that this code is very bad and you should use special functions for comparison.

Another example:

if (lpszHelpFile != 0)
{
  pwzHelpFile = ((_lpa_ex = lpszHelpFile) == 0) ?
0 : Foo(lpszHelpFile);
  ...
}

This code works quite correctly but it is too tangled. The "((_lpa_ex = lpszHelpFile) == 0)" condition is always false, as the lpszHelpFile pointer is always not equal to zero. This code is difficult to read and should be rewritten.

This is the simplified code:

if (lpszHelpFile != 0)
{
  _lpa_ex = lpszHelpFile;
  pwzHelpFile = Foo(lpszHelpFile);
  ...
}

Another example:

SOCKET csd;
csd = accept(nsd, (struct sockaddr *) &sa_client, &clen);
if (csd < 0)
  ....

The accept function in Visual Studio header files returns a value that has the unsigned SOCKET type. That's why the check 'csd < 0' is invalid since its result is always false. The returned values must be explicitly compared to different constants, for instance, SOCKET_ERROR:

if (csd == SOCKET_ERROR)

The analyzer warns you far not of all the conditions which are always false or true. It diagnoses only those cases when an error is highly probable. Let's consider some samples that the analyzer considers absolutely correct:

// 1) Eternal loop
while (true)
{
...
}

// 2) Macro expanded in the Release version
// MY_DEBUG_LOG("X=", x);
0 && ("X=", x);

// 3) assert(false);
if (error) {
  assert(false);
  return -1;
}
You can look at examples of errors from real projects which were detected by this diagnostic message.