[optional] Counterintuitive comparators

I was recently bitten by bug in some code using boost::optional. A simple test case is: #include <boost/optional.hpp> #include <cassert> int main() { boost::optional<int> i = 0; assert( i == 0 ); } Somebody had written code like this, presumably assuming the second argument to the comparison operator would convert implicitly to a boost::optional<int> and the code would behave as if it read: assert( i && i.get() == 0 ); Unfortunately, although the code compiles, overload resolution finds an additional undocumented operator: bool boost::operator==<int>( boost::optional<int> const&, boost::none_t const& ) This is selected because none_t is a pointer-to-member type and 0 is a valid null pointer-to-member literal, meaning the code is equivalent to assert( not i ); which asserts. Even in the absense of this operator, the comparison preferetially goes via i's conversion to unspecified-bool-type (again, implemented as a pointer-to-member function) and the code asserts. (In both cases, if '0' is replaced by '1' -- no longer a null pointer literal -- the code fails at compile time.) In this respect, boost::optional is the same as, say, boost::shared_ptr, in that C++'s lack of less conversion- prone boolean type can sometimes cause odd effects. However, where boost::optional has value-semantics, is implicitly constructible from T, and has 'deep' comparision semantics, unlike boost::shared_ptr, users are much more likely accidently to write code that is buggy because of this. I've been trying to think of a way of triggering a compile-time error in this case, but I haven't been able to think of anything wholly satisfactory. The best way I can do is to have comparison operators that trigger a BOOST_STATIC_ASSERT: template <class T, class NullPointerType> typename boost::enable_if< boost::is_integral< NullPointerType >, bool
::type operator==( boost::optional<T> const& x, NullPointerType y ) { BOOST_STATIC_ASSERT( false ); }
(and similarly for the other comparisons, and with the arguments transposed). This could be wrapped up into a 'safe_bool_conversion' base class: // Implement a safe boolen conversion using operator! template <class Derived> class safe_bool_conversion { void dummy_fn() const {} typedef void (safe_bool_conversion::* bool_t)() const; public: operator bool_t() const { return !!static_cast<Derived const&>(*this); } // Plus the various asserting operators }; template <class T> class optional : public safe_bool_conversion< optional<T> > { }; An alternative (for boost::optional) would be to simply create valid operators for each of these comparisons so that the original code does the expected thing. template <typename T, typename U> inline typename boost::enable_if< boost::is_convertible<U,T>, bool
::type operator==( boost::optional<T> const& x, U const& y ) { return x && x.get() == y; }
... and so on. Does this sound worth persuing? Richard Smith

Richard Smith wrote:
I was recently bitten by bug in some code using boost::optional. A simple test case is:
#include <boost/optional.hpp> #include <cassert>
int main() { boost::optional<int> i = 0; assert( i == 0 ); }
Somebody had written code like this, presumably assuming the second argument to the comparison operator would convert implicitly to a boost::optional<int> and the code would behave as if it read:
assert( i && i.get() == 0 );
OK, deep comparison operators have been requested before, but all this time I remained unconvinced there were really needed, until now. As you said, optional<> provides value-based semantics in other places, so given the kind of bug you discovered, I think is time to add these too. Best Fernando Cacciola
participants (2)
-
Fernando Cacciola
-
Richard Smith