V547. Expression is always true/false.


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

Note. Every now and then, we get similar emails where users tell us they don't understand the V547 diagnostic. Let's make things clear. This is the typical scenario described in those emails:

for (int i = 0; i <= 1; i++)
{
  if(i == 0)
    A();
  else if(i == 1)        // V547
    B();
}

The analyzer issues the warning "Expression 'i == 1' is always true", but it's not actually true. The value of the variable can be not only one but also zero. Perhaps you should fix the diagnostic.

Explanation. The warning doesn't say that the value of the 'i' variable is always 1. It says that 'i' equals 1 in a particular line and points this line out.

When executing the check 'if (i == 1)', it is known for sure that the 'i' variable will be equal to 1. There are no other options. This code is of course not necessarily faulty, but it is definitely worth reviewing.

As you can see, the warning for this code is absolutely legal. If you encounter a warning like that, there are two ways to deal with it:

  • If it's a bug, fix it.
  • If it's not a bug but just an unnecessary check, remove it.

Simplified code:

for (int i = 0; i <= 1; i++)
{
  if(i == 0)
    A();
  else
    B();
}

If it's an unnecessary check, but you still don't want to change the code, use one of the false positive suppression options.

Additional materials on this topic:

  • An interesting example, when the V547 warning seems to be strange and incorrect. But if you study this out, it turns out that the code is actually dangerous. Discussion at StackOverflow: Does PVS-Studio know about Unicode chars?


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;
We use cookies for the analysis of events to improve our content and make user interaction more convenient. By continuing the view of our web-pages you accept the terms of using these files. You can find out more about cookie-files and privacy policy or close the notification, by clicking on the button. Learn More →
Do not show