15. Start using enum class in your code, if possible

All the examples of this error I have are large. I've picked one of the smallest, but it's still quite lengthy. Sorry for that.

This bug was found in Source SDK library. The error is detected by the following PVS-Studio diagnostic: V556 The values of different enum types are compared: Reason == PUNTED_BY_CANNON.

enum PhysGunPickup_t
{
  PICKED_UP_BY_CANNON,
  PUNTED_BY_CANNON,
  PICKED_UP_BY_PLAYER,
};

enum PhysGunDrop_t { DROPPED_BY_PLAYER, THROWN_BY_PLAYER, DROPPED_BY_CANNON, LAUNCHED_BY_CANNON, };

void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason) { .... if( Reason == PUNTED_BY_CANNON ) { PlayPuntSound(); } .... }</pre>

Explanation

The Reason variable is an enumeration of the PhysGunDrop_t type. This variable is compared to the named constant PUNTED_BY_CANNON belonging to another enumeration, this comparison being obviously a logical error.

This bug pattern is quite widespread. I came across it even in such projects as Clang, TortoiseGit, and Linux Kernel.

The reason why it is so frequent is that enumerations are not type safe in the standard C++; you may get easily confused about what should be compared with what.

Correct code

I don't know for sure what the correct version of this code should look like. My guess is that PUNTED_BY_CANNON should be replaced with DROPPED_BY_CANNON or LAUNCHED_BY_CANNON. Let it be LAUNCHED_BY_CANNON.

if( Reason == LAUNCHED_BY_CANNON )
{
  PlayPuntSound(); 
}

Recommendation

Consider yourself lucky if you write in C++; I recommend that you start using enum class right now and the compiler won't let you compare values, that refer to different enumerations. You won't be comparing pounds with inches anymore.

There are certain innovations in C++ I don't have much confidence in. Take, for instance, the auto keyword. I believe it may be harmful when used too often. Here's how I see it: programmers spend more time reading the code rather than writing it, so we must ensure that the program text is easy-to-read. In the C language, variables are declared in the beginning of the function, so when you edit the code in the middle or at the end of it, it's not always easy to figure what some Alice variable actually means. That's why there exists a variety of variable naming notations. For instance, there is a prefix notation, where pfAlice may stand for a "pointer to float".

In C++, you can declare variables whenever you need, and it is considered a good style. Using prefixes and suffixes in variable names is no longer popular. And here the auto keyword emerges, resulting in programmers starting to use multiple mysterious constructs of the "auto Alice = Foo();" kind again. Alice, who the fuck is Alice?!

Sorry for digressing from our subject. I wanted to show you that some of the new features may do both good and bad. But it's not the case with enum class: I do believe it does only good.

When using enum class, you must explicitly specify to which enumeration a named constant belongs to. It protects the code from new errors. That is, the code will look like this:

enum class PhysGunDrop_t
{
  DROPPED_BY_PLAYER,
  THROWN_BY_PLAYER,
  DROPPED_BY_CANNON,
  LAUNCHED_BY_CANNON,
};

void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason) { .... if( Reason == PhysGunDrop_t::LAUNCHED_BY_CANNON ) { PlayPuntSound(); } .... }</pre>

True, fixing old code may involve certain difficulties. But I do urge you to start using enum class in new code right from this day on. Your project will only benefit from it.

I don't see much point in introducing enum class. Here's a few links for you to learn all the details about this new wonderful feature of the C++11 language:

1. Wikipedia. C++11. Strongly typed enumerations.
2. Cppreference. Enumeration declaration.
3. StackOverflow. Why is enum class preferred over plain enum?