Having checked a large number of open source projects, the team at PVS-studio has seen a large variety of ways to shoot yourself in the foot. Drawing from the C++ Hints website, Andrey Karpov has compiled a list of 42 tips, four of which are reproduced here. While intended primarily for C/C++ programmers, usually they are universal, and may be of interest for developers using other languages. 

Larger than 0 does not mean 1

The following code fragment, taken from CoreCLR project, has an error that PVS-Studio analyzer diagnoses in the following way:

V698 Expression 'memcmp(....) == -1' is incorrect. This function can return not only the value '-1', but any negative value. Consider using 'memcmp(....) < 0' instead.

bool operator( )(const GUID& _Key1, const GUID& _Key2) const
  { return memcmp(&_Key1, &_Key2, sizeof(GUID)) == -1; }

Explanation

Let's have a look at the description of memcmp() function:

int memcmp ( const void * ptr1, const void * ptr2, size_t num );

It compares the first num bytes of the block of memory pointed by ptr1 to the first num bytes pointed by ptr2, returning zero if they all match, or a value different from zero representing which is greater, if they do not.

Return values:

• < 0 - the first byte that does not match in both memory blocks has a lower value in ptr1 than in ptr2 (if evaluated as unsigned char values)
  
  
• == 0 - the contents of both memory blocks are equal
  
  
• > 0 - the first byte that does not match in both memory blocks has a greater value in ptr1 than in ptr2 (if evaluated as unsigned char values)

Note that if blocks aren't the same, then the function returns values greater than or less than zero. Greater or less. This is important! You cannot compare the results of such functions as memcmp(), strcmp(),  strncmp(), and so on with the constants 1 and -1.

Interestingly, the wrong code, where the result is compared with the 1/-1 can work as the programmer expects for many years. But this is sheer luck, nothing more. The behavior of the function can unexpectedly change. For example, you may change the compiler, or the developers will optimize memcmp() in a new way, so your code will cease working.

Correct code

bool operator( )(const GUID& _Key1, const GUID& _Key2) const
  { return memcmp(&_Key1, &_Key2, sizeof(GUID)) < 0; }

Recommendation

Don't rely on the way the function works now. If the documentation says that a function can return values less than or greater than 0, it does mean it. It means that the function can return -10, 2, or 1024. The fact that you always see it return -1, 0, or 1 doesn't prove anything.

By the way, the fact that the function can return such numbers as 1024, indicates, that the result ofmemcmp() execution cannot be stored in the variable of char type. This is one more widespread error, whose consequences can be really serious. Such a mistake was the root of a serious vulnerability in MySQL/MariaDB in versions earlier than 5.1.61, 5.2.11, 5.3.5, 5.5.22. The thing is, that when a user connects to MySQL/MariaDB, the code evaluates a token (SHA from the password and hash) that is then compared with the expected value of memcmp() function. But on some platforms the return value can go beyond the range [-128..127] As a result, in 1 out of 256 cases the procedure of comparing hash with an expected value always returns true, regardless of the hash. Therefore, a simple command on bash gives a hacker root access to the volatile MySQL server, even if the person doesn't know the password. The reason for this was the following code in the file 'sql/password.c':

typedef char my_bool;
...
my_bool check(...) {
  return memcmp(...);
}

"Look what I can do!" - Unacceptable in programming

Sometimes it feels like programmers are competing against somebody, trying to write the shortest code possible. This is a situation which is relevant for both C and C++ programmers. They tend to make the constructions more complicated, thinking, "I do it because I can".

The fragment is taken from KDE4 project. The error is detected by the following PVS-Studio diagnostic:

V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'.

void LDAPProtocol::del( const KUrl &_url, bool )
{
  ....
  if ( (id = mOp.del( usrc.dn() ) == -1) ) {
    LDAPErr();
    return;
  }
  ret = mOp.waitForResult( id, -1 );
  ....
}

Explanation

After looking at this code, my questions are; What was the point of doing it? Did you want to save a line? Did you want to show that you can combine several actions in one expression?

As a result we have a typical error pattern - using expressions of the if (A = Foo() == Error) kind.

The precedence of the comparison operation is higher than that of the assignment operation. That's why the "mOp.del( usrc.dn() ) == -1" comparison is executed first, and only then the true (1) or false (0) value is assigned to the id variable.

If mOp.del() returns '-1', the function will terminate; otherwise, it will keep running and the 'id' variable will be assigned an incorrect value. So it will always equal 0.

Correct code

I want to emphasize: adding extra parentheses is not a solution to the problem. Yes, the error can be eliminated. But it's the wrong way.

There were additional parentheses in the code - have a closer look. It's difficult to say what they were meant for; perhaps the programmer wanted to get rid of the compiler warnings. Perhaps he suspected that the operation priority may be not right, and wanted to fix this issue, but failed to do so. Anyway, those extra brackets don't help.

There is a deeper problem here. If it is a possible not to make the code more complicated, don't. It is better to write:

id = mOp.del(usrc.dn());
if ( id == -1 ) {

Recommendation

Don't be so lazy as not to write an extra code line: complex expressions are hard to read, after all. Do the assignment first, and only then, the comparison. Thus you will make it easier for programmers who will be maintaining your code later, and also it will reduce the chances of making a mistake.

So my conclusion is - don't try to show off.

This tip sounds trivial, but I hope it will help you. It's always better to write clear and neat code, instead of in a "see how cool I am" style.