isocpp / CppCoreGuidelines

The C++ Core Guidelines are a set of tried-and-true guidelines, rules, and best practices about coding in C++
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines
Other
42.89k stars 5.44k forks source link

Suggestion: ES.63: Distinguish between logical and bitwise comparison #502

Open richelbilderbeek opened 8 years ago

richelbilderbeek commented 8 years ago

Edit: Added text to Exceptions section, due to @BjarneStroustrup's comment

ES.63: Distinguish between logical and bitwise comparison

Reason:

Say what you mean. If you want both the first and second term to be true, use operator&&. If you want to match bitflags, use operator&.

Matching bitflags can only be done sensibly using operator&, where doing a logical comparison can be done using one and two ampersands. In this case, use operator&&. There is no run-time speed to be gained using operator& instead.

The same story applies to operator| and operator||.

Examples:

Both terms being some value:

if (a == 42 && b == 3.14) { /* */ } //OK
if (a == 42 & b == 3.14) { /* */ } //Bad, will give compiler warning
if ((a == 42) & (b == 3.14)) { /* */ } //Bad, say what you mean

Bitflag:

if (i & flag_is_visible) { /* draw */ } //OK
if (i && flag_is_visible) { /* draw */ } //Runtime error

Alternatives: none

Exceptions:

If you don't need the order dependence of && and you've measured that & is a significant performance optimization. If this is the case, it should be marked as an optimization. One way to do so is by making it an inline function, e,g, fast_and(0<=a,a<max).

Enforcement: compilers already give warnings

See alsos: none

Notes:

I saw the a == 42 & b == 3.14 in the wild, that's why I suggested this rule, as the current guidelines do not state the preferred form explicitly yet.

Discussion:

Perhaps this item would better be renamed to a Non-Rule: NR x: operator& does not outperform operator&& in runtime performance

TBBle commented 8 years ago

There's also a behaviour difference in short-circuit evaluation. For a trivial literal example, it doesn't matter, but if the user was trying to force both expressions to be evaluated, then operator& would do what they want, at the cost of being completely unreadable to the next person.

So probably worth calling out in such a Non-Rule that avoiding short-circuit evaluation is not a good reason to use operator& with bool values.

nadiasvertex commented 8 years ago

Any attempts to avoid short-circuit evaluation are misguided at best. Maybe that is another candidate for a non-rule. If you need the side-effects of an evaluation, do it explicitly, and call that out. Otherwise it might get optimized away by a maintenance cycle.

On Sun, Jan 17, 2016 at 7:16 PM TBBle notifications@github.com wrote:

There's also a behaviour difference in short-circuit evaluation. For a trivial literal example, it doesn't matter, but if the user was trying to force both expressions to be evaluated, then operator& would do what they want, at the cost of being completely unreadable to the next person.

So probably worth calling out in such a Non-Rule that avoiding short-circuit evaluation is not a good reason to use operator& with boolean values.

— Reply to this email directly or view it on GitHub https://github.com/isocpp/CppCoreGuidelines/issues/502#issuecomment-172398654 .

BjarneStroustrup commented 8 years ago

If you don't need the order dependence of &&, you can use & and that can be a significant performance optimization. I think that what we should say is that if you use bitwise operations only to avoid the implied branch in &&, it should be marked as an optimization, possibly by making it an inline function, e,g, fast_and(0<=a,a<max).

drearyworlds commented 8 years ago

As a possible exception to this, I use &= for Boolean comparison sometimes. E.g.:

bool success = true; success &= (condition1); //other code success &= (condition2); return success;

As there is no &&= operator.

excaliboor commented 8 years ago

@drearyworlds What's wrong with

success = success && (condition);

The LHS OP= RHS is a shortcut for LHS = LHS OP RHS anyway

richelbilderbeek commented 7 years ago

I have added text to the Exceptions section, due to @BjarneStroustrup's comment. OTOH, this only takes that comment partially into account.

Perhaps, instead of adding the comment as an exception, it should become a rule of its own, for example:

ES 64. When using bitwise operations only to avoid the implied branch in &&, it should be marked as an optimization