unsafe global_ptr bool conversion

Issue #202 resolved
Dan Bonachea created an issue

Our implementation of global_ptr currently contains the following member function:

    operator bool() const {
      return raw_ptr_ != nullptr;
    }

This enables the end-user to write handy things like the following, which people generally expect from a pointer-like class:

  global_ptr<int> gp = upcxx::new_<int>();
  if (gp)  cout << "true" << endl;
  if (!gp) cout << "false" << endl;
  else     cout << "true" << endl;

This usage pattern is not currently required to work by the specification, but it seems a sufficiently useful pattern that we should consider guaranteeing it will work.

Unfortunately, the use of operator bool() is a low-quality implementation of this idiom. In particular, it allows all the following surprising/erroneous behavior that we really don't want:

  int x = gp; // should really be a type error, but is not!
  gp << 1; // this should also be a type error, but is not!

  // this should really be a type error.
  // instead, it not only compiles but the equality test passes!!!
  global_ptr<double> gpd = upcxx::new_<double>();
  if (gp == gpd) cout << "OOPS" << endl;

We really want the compiler to reject all the code above, but it does not (tested with gcc 5.4.0 and 8.2.0).
The comparison in the last line is especially problematic: it not only compiles without complaint, but gives a very unintuitive result - the comparison appears to be testing pointer equality, but the condition it's actually testing is whether both pointers are non-null (or both are null). The problem is fairly obvious in this tiny snippet, but when the pointer declarations are in separate files this could be a very difficult bug to track down. This defect is present in both the develop head and the current public release.

Luckily, there is a better way to accomplish what we want here: The Safe Bool Idiom
it's specifically designed to provide the boolean testing idiom without introducing the problems described above.

We should investigate adapting the Safe Bool idiom to global_ptr (with minor modifications, since we need correctly-typed pointer comparisons to work).

Comments (3)

  1. Amir Kamil

    The C++11 solution is to make the operator explicit.

    This conversion isn't in the spec; do we want it there?

  2. Dan Bonachea reporter

    This conversion isn't in the spec; do we want it there?

    I would very much like to see the boolean idiom specced (and I suspect there is already code relying upon it), although I'm not sure the spec should mandate an implementation.

  3. Log in to comment