assert in move assign operator

[J-Richter]

The operator MoveAssignmentBase & operator=(MoveAssignmentBase && rhs) has this assert:

assert(this->has_value() == rhs.has_value());

It triggers in our code. The reason is that

this->AssignValue(std::move(rhs.GetPayload()));

might set rhs to the empty state if the moved-from-state happens to be the empty-state. I don't know what the assert is supposed to protect. It looks unnecessary. Could there be problems if I remove it?

[Sedeniono]

According to the C++ standard (i.e. in case of std::optional), the moved-from optional is required to still contain a value after the move. I guess that is the reason why I added the assert, without realizing at that time that this condition can be violated for tiny::optional. The same holds for the move-constructor, but there I apparently forgot a similar assert.

I unfortunately never realized this kind of "interaction" between moving-from and the empty state for more complex types. This can cause troubles in the general case. Imagine a type T containing an integer i and some more complex object, let's say a std::unorderd_map m. Further assume that i==0 is used to indicate the empty state. If the move constructor/assignment operator of the T sets i to 0, the moved from tiny::optional will then be thought of as empty. This in turn might cause tiny::optional to never properly call the destructor of the type T and thus of m, causing undefined behavior (e.g. a memory leak of m).

I will have to think about this problem. Since I am currently away, I won't be able to work on this for the next week.

But if the move constructor/assignment operator of your type destroys any non-trivially-destructible members, or if it contains only trivially-destructible-members (std::is_trivially_destructible), I think you are fine and the assert can be simply removed.

[Sedeniono]

I had a closer look. Whenever the payload of the optional changes somehow "externally" without the optional's reset function or assignment with nullopt involved (e.g. via a move assignment that causes the isEmpty-flag to become true, or calling any other member on the payload that has this effect, or a v = std::move(myOptional.value())), there is a potential problem. The problem is that when destroying the payload of a non-empty optional, the type's destructor is called. But for empty optionals with a specialized tiny::optional_flag_manipulator, the destruction is now done in tiny::optional_flag_manipulator::invalidate_empty_flag(). So far, invalidate_empty_flag() is not required to call the destructor of the whole payload type. If it does call the ordinary destructor, then everything is fine. But if it doesn't, we have undefined behavior, resulting in a memory leak. The reason is that the optional actually still contains the full payload object despite being seen as empty, and thus the destructor of the full payload type needs to get called.

The idea with invalidate_empty_flag() and init_empty_flag() was that it would be possible to not create always the full payload type but also only a part of it. I.e. not call the payload's constructor (and initialize its members to whatever the user defined as empty state), but essentially construct only a specific member of the payload and keeping the remaining bits & bytes in an undefined state. This allows for more flexibility, but is actually relying on undefined behavior. (Because is_empty() is called for empty and non-empty optionals, and we deal with two distinct types, i.e. we have type aliasing.)

My best idea currently to fix the issues is to require that tiny::optional_flag_manipulator::init_empty_flag() always constructs a full payload (i.e. does a placement new for the full object), and that tiny::optional_flag_manipulator::invalidate_empty_flag() always calls the destructor of the full payload type. Never for only parts of the payload. Of course, the latter could also be done by tiny::optional itself, making tiny::optional_flag_manipulator::invalidate_empty_flag() obsolete.

The debug assert in the move-assignment would then be wrong. tiny::optional would then behave differently than std::optional by design (the moved-from optional could be empty afterwards).

@J-Richter Would this change of API cause problems for you? Do you use types where tiny::optional_flag_manipulator::invalidate_empty_flag() does not simply call the destructor of the full type? If yes, could you provide an example? Thanks!

[J-Richter]

I use a class that is similar to the following OddInt class.
Basically you have a class with a wide range of possible values (Int). Then you have another class which restricts the possible values (OddInt). And tiny-optional reused one of the now unused values (0).

Here the empty state (value_==0) cannot be constructed with the official interface of the OddInt class.

#include "tinyoptional.h"
#include <bld/initialize.h>
#include <cstddef>
#include <cstring>
#include <cstdlib>
#include <utility>

class Int
{
    int value_;

  public:
    Int( int val )
      : value_( val )
    {}

    Int( Int const& rhs )
      : value_( rhs.value_ )
    {}

    Int( Int && rhs )
      : value_( rhs.value_ )
    {
      rhs.value_ = 0;
    }

    Int& operator=( Int val )
    {
      value_ = val.value_;
      return *this;
    }

    int getValue() const
    {
      return value_;
    }
};

class OddInt
{
    Int value_;

  public:
    OddInt( Int value )
      : value_( value )
    {
      assert( value_.getValue() & 1 );
    }
};

template<> struct tiny::optional_flag_manipulator<OddInt>
{
    static bool is_empty( OddInt const& n ) noexcept
    {
      return reinterpret_cast<Int const*>( &n )->getValue() == 0;
    }

    static void init_empty_flag( OddInt& n ) noexcept
    {
      ::new( &n ) Int( 0 );
    }

    static void invalidate_empty_flag( OddInt& n ) noexcept
    {
      reinterpret_cast<Int*>( &n )->~Int();
    }
};

int main( int, char** )
{
  tiny::optional<OddInt> s1;
  s1 = OddInt( Int( 45 ) );
  tiny::optional<OddInt> s2;
  s2 = std::move( s1 );
}

I think it cannot happen that the object is changed externally, at least with move. Even moving the contained value like move( *s1 ) will never set the integer to 0. This will only happen if two optionals are involved. But then only the responsibility for the destruction is exchanged and all objects are still destroyed properly.

However, I am not quite sure. Maybe you can think of another counterexample.

I think it would be bad if you lose the ability to construct only a part of the object. This is one of the strong points of this library.

[J-Richter]

After thinking a bit more about the issue, I think that there may not be a problem here after all. In my example, the OddInt class does not have a correct move constructor. I.e. its value is no longer odd after a move. If this problem is fixed, the assert doesn't hit anymore.

A similar problem exists in the original class in my source code. After I fixed it, the assert doesn't hit anymore either.
So I think this issue can be closed if you share the same opinion.

[Sedeniono]

After thinking some more days about this problem, I failed to come up with a reasonable solution to support payload types in general where some of its member function causes a transition to the empty state (e.g. move assignment operator or constructor; except of course the function used by the init_empty_flag() specialization).
It would be possible to add support for such types for the tiny::optional move constructor and assignment operator. And also to assign a payload object currently indicating the empty state to tiny::optional (instead of e.g. std::nullopt). But everything fails with code such as

tiny::optional<T> o = ...; // non empty o
o.value().MakeEmpty();

or also:

tiny::optional<T> o = ...;  // non empty o
T t = std::move(*o); // Calls move assignment operator of T

assuming that T::MakeEmpty() and the move assignment operator of T cause a transition of T to the state which tells tiny::optional that it is empty. The transition happens entirely outside of any tiny::optional code, and thus by its very nature cannot be detected. (Ok, maybe by adding an additional variable to tiny::optional to remember the previous state and compare with it; but this would increase the optional's size, defeating the whole purpose of this library.)

So, I pushed a commit (4c9e940) and new release (v1.1.1):

• that adds the missing debug assert also to the move constructor of tiny::optional (in analogy to the move assignment operator that you ran into).
• explained the issue in the readme, with the following guideline:

  As a guideline, ensure that the payload can transition to the state which indicates emptiness ONLY by calling init_empty_flag(). The empty state should not be constructable via the payload's default constructor, move constructor, move assignment operator and any other member function, except possible a single dedicated one that is used exclusively by the init_empty_flag() specialization.