Flags, bitwise operators and conversion to flag groups

[chris-durand]

Instances of FlagsOperators, which are created when Flags are combined using bitwise operators, do not implicitly convert to a FlagsGroup.
However, FlagsOperators convert to Flags which themselves implicitly convert to FlagsGroup. Since the compiler will never apply more than one user-defined conversion automatically, you need an explicit cast.

Adding a FlagsOperators conversion constructor to the FlagsGroup class template should eliminate unnecessary casts without allowing additional undesired conversions.
Here you can find an implementation.

Did I miss some unintended side effects?

[salkinium]

.oO( Le sigh, C++ is such a terrible language. )

I remember adding FlagsOperators rather hastily, so it's very likely I missed this.
The best way to make sure this does what it's supposed to do is to add a unittest that fails before this fix is applied, and passes afterwards.

[chris-durand]

The best way to make sure this does what it's supposed to do is to add a unittest that fails before this fix is applied, and passes afterwards.

I have no idea how to write a reasonable unittest for this feature. This is a language issue during compilation. Before the fix the code would simply refuse to compile instead of failing at runtime.

[salkinium]

Well, do you have an example of code that compiles with this change, but doesn't without it? That's your unittest.

[chris-durand]

Well, do you have an example of code that compiles with this change, but doesn't without it? That's your unittest.

I'll add the test soon.

.oO( Le sigh, C++ is such a terrible language. )

It will get even better... I encountered another issue after adding the suggested conversion operators. In some driver code I defined two function overloads of which one takes a FlagsGroup and the other one takes a uint8_t. Everything works as intended unless you dare to pass bitwise or-ed flags to it. The compiler complains about the overload being ambiguous although it is clear the flags don't implicitly convert to uint8_t.

Here is where the fun part starts. All Flag* classes inherit from Register which defines explicitly deleted int conversion operators to prevent unintentional implicit casts to int types through the bool conversion operator. The overload resolution fails because the compiler sees the deleted uint8_t conversion operator.

How can that be? We explicitly told it: Do not convert to uint8_t. C++ wouldn't be C++ if it weren't much more complicated ;-). When you write " = delete" you delete the definition of the operator but not the symbol (see this for more details). During overload resolution the delete isn't even noticed. By prohibiting conversion to a specific type you urge the compiler to try it. Thank you C++.

This is kind of a corner case and could be resolved by adding a cast in my code, but in my opinion library code should not behave unexpectedly when it's avoidable.
The only solution is to find an implementation of the implicit bool conversion in the Register class that does not rely on explicitly deleted conversion operators.

The classic C++03 solution is the rather clumsy safe bool idiom.

In C++11 a concept called "contextual conversion" was introduced for types which explicitly convert to bool. It will make an explicit bool conversion operator behave as if it was implemented using the safe bool idiom: It will never convert to int implicitly but will trigger an "implicit explicit cast" when used with if, for, logical operators and so on (See this for a more complete list).

Thus it would be possible to make the bool operator explicit and entirely remove the deleted conversion operators. Unfortunately there is one drawback. In return statements of functions returning bool no contextual conversion takes place and an explicit cast is needed. So employing an explicit bool operator would change the external interface of the flags classes. This is not an option since user code may rely on that.

Finally I suggest either to use the safe bool idiom in the Register class or to do nothing at all. What do you think?

[salkinium]

This is pretty amazing research, thanks for taking the time! This is a bit shocking to me.

By prohibiting conversion to a specific type you urge the compiler to try it.

I literally molled (moaned out loud) at this.

Finally I suggest either to use the safe bool idiom in the Register class or to do nothing at all. What do you think?

I'd be in favour of removing both operator bool() and operator!() and replacing it with a function isNull(). I think it's bad design to implicitly convert a register to bool, since we're already doing everything to make it behave not like an integer.

We used to delete all the casting operators with a template, but it didn't compile on hosted gcc so we stopped. Not sure if that would be helpful here.
Do you know if switching to C++14 would improve any of this?

[chris-durand]

We used to delete all the casting operators with a template, but it didn't compile on hosted gcc so we stopped. Not sure if that would be helpful here.

I don't think deleted casting operators are helping here at all. They are used here to heal the insane default behaviour of the non-explicit version of operator bool() and have surprising side effects with function overloading. You provided a good example for that yourself. Once you added the deleted template conversion operator it matched all function overloads. This is not a bug in GCC as assumed in the comment in the source code but the standard compliant behaviour. That operator basically means: pretend to convert to everything and fail when the conversion is actually attempted.

I'd be in favour of removing both operator bool() and operator!() and replacing it with a function isNull(). I think it's bad design to implicitly convert a register to bool, since we're already doing everything to make it behave not like an integer.

Please don't do that. If you wanted to check whether a flag is set you would have to write something like if( not (flag & Flag1::A).isNull()) instead of if(flag & Flag1::A). I don't see how that is an improvement.

It's true bool conversion operators can be harmful when they allow undesired conversions or have confusing semantics. For instance in the C++ standard library input streams have bool operators. A true value means that no error occurred but not that data can be read. It will return true even if the end of a file has been reached.

Here both isn't the case. In my opinion it is a key advantage of this flags implementation that it is both type safe and retains the usual semantics of bit operations.

If it's acceptable to subtly change the interface of the flags classes I would recommend using the C++11 explicit bool operator. You can completely remove all deleted cast operators and even don't need to define operator!() anymore. The explicit operator is more restrictive than the safe bool solution. It won't allow any conversions to int types, assignments to bool without a cast and won't interfere with function overloading, but still allows for code like the if statement above.
The required changes in XPCC are minimal. We would only need to change ~5 lines where bool conversion are used in a return statement.

Do you know if switching to C++14 would improve any of this?

I'm not aware of any feature that would help here.

[salkinium]

Please don't do that.

Yeah, good point, that's disgusting.

If it's acceptable to subtly change the interface of the flags classes I would recommend using the C++11 explicit bool operator.

Yes, let's go for that!

[chris-durand]

Well, do you have an example of code that compiles with this change, but doesn't without it? That's your unittest.

I added a simple test that fails to compile without the fix.

While I was writing the test I found out that the conversion problem is more subtle than I initially expected. At first I combined 2 enum constants directly and the code surprisingly compiled. It compiled because the resulting type is Flags<Enum,uint_8> which is perfectly convertible to the flags group. However, the code fails to compile when you combine 3 enum constants or involve a variable of type Flags. Then the resulting type is actually FlagsOperators<Enum,uint_8>. This is totally confusing.

Yes, let's go for that!

The change in register.hpp is really trivial but to definitively catch every other line that has to be changed I have to go through all code that uses flags and is not compiled for unit tests or the examples.

When I'm ready I'll create a pull request.

[salkinium]

Fixed in #230.