[boost] [optional] Counterintuitive comparators

From: Richard Smith (richard_at_[hidden])
Date: 2006-10-23 13:30:20

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