C++ std::optional implementation for tech interview

Here is the list of issues I've found in the implementation in no particular order:

The operator bool should be explicit, otherwise it's allowed to implicitly compare the optional with a boolean, which is a source of bugs.

The value() functions are expected to throw if a value is not stored.

The class should define value_type just as std::optional does.

The class is always advertised as copy-constructible and copy-assignable, but in practice, it is not when T is not copy-constructible or copy-assignable. Meaning that all SFINAE and concept checks will always assume it is copy-constructible and copy-assignable even when it isn't. std::optional even treats non-move-constructible and non-move-assignable classes properly at least according to documentation in cppreference.

Copy constructors and copy assignment are usually not needed to be noexcept. You also lack converting-copy-assignment and converting-move-assignment, i.e.,

template< class U >
optional& operator=( const optional<U>& other );

Your move-constructor and move-assignment don't match what std::optional does. std::optional doesn't perform a "steal," it only forward the move operation onto the underlying object.

std::optional has swap function.

Where is the analogy of std::nullopt_t and std::nullopt? Or at least their usage in the class?

Also, see constructors (7) and (8) in the link, you don't have them implemented.

You should adequately restrict the emplace function to only being available when T is constructible from the arguments.

The class ought to have comparison operations when T does.

In C++23, the class got monadic operations or_else, transform, and_then.

As an interviewer, I'm interested in your development process. So it's really valuable to see evidence of how you tested your code, preferably as a suite of unit tests. Without those, not only will I need to work harder, but I'll also be much more wary of the correctness.

Evidence of testing is of course helpful in any code review, so I encourage you to always present the tests along with the code. When reviewers can see what's been proved to be functionally correct (and more importantly, what has been missed from testing), then they can spend more time on the aspects that benefit from human inspection.

When implementing a type with a published interface, I believe it's important to implement exactly the interface that is specified. In this case, we've failed at pretty much the first hurdle:

optional(T&& value) noexcept;

This accepts only an rvalue, so there's no match here:

    int a = 0;
    optional<int> oa = a;  // compilation error

The standard type's constructor accepts a forwarding reference, like this:

/* simplified */
template<class U> optional(U&& v);

Here, U can bind to any value or reference type, such as the lvalue a above.

Consider giving _engaged an in-class initialiser. Then we could write the default constructor as optional() noexcept = default.

It's good to see noexcept being used. However, I think it has been sprinkled a little too liberally. For example, the constructors assign a T&& to the _value member, which can throw unless std::std::is_nothrow_move_constructible_v<T>.

Similarly, the assignment operators of std::optional are specified to be noexcept(is_move_constructible_v<T> && is_move_assignable_v<T>).

In the specification, pretty much all of the interface is declared constexpr, but this code makes no attempt at that.

Another departure from specification is that conversions to T& or T&& are required to throw std::bad_optional_access if there's no contained value, but we just have the same UB as in operator*().

We're completely missing an implementation corresponding to std::optional::swap().

We're also missing the non-member parts of the interface (relational operators, make_optional(), nullopt_t and std::swap<optional>).

I find it surprising that we use std::construct_at() to create values, but use plain ~T() rather than the consistent std::destroy_at() in our reset().

A final note: be careful with identifiers beginning with underscore, particularly in places that also use the C language. While you might be fully conversant with the rules, your interviewer might not be; given the prospect of an in-depth language-lawyer argument, it might be better to deflect that well in advance.

        if constexpr (!std::is_trivially_destructible_v<T>) {
            _value.~T();
        }

why would you "if ~T() does nothing, skip calling ~T()"?

        _engaged = false;

should you _engaged = false before or after you destroy the object? During destruction, of the contained object in an optional, should the optional appear engaged or not?

if (this != &rhs) {

what goes wrong with your assignment implementations if this is false? I think nothing. Lets simplify!

optional& operator=(const optional& rhs) /*& would be good, but std lacks it */
                                         noexcept(/* needs a condition*/)
{
  if (rhs._engaged && _engaged) {
    _value = rhs._value; // self assigment is ok
  } else if (rhs._engaged) { // impossible for self-assignment
    // the construction case:
    std::construct_at(std::addressof(_value), rhs._value);
    _engaged = true;
  } else {
    reset();
  }
  return *this;
}

I think that is simpler? It also means that if T=T is non-trivial for the same object T, we actually call it.

For this to be noexcept, you need all of destruction, assignment and construction to be noexcept.

T& emplace(Args... args) {

this should be Args&&... to perfect forward. In addition, I personally would make it the central point of creating T objects.

To do this,

bool _engaged = false;

so that it is in good shape "by default"; DRY, say it once.

optional(T&& value) noexcept : _value(std::move(value)), _engaged(true) {}

becomes

optional(T&& value) noexcept {
  emplace(std::move(value));
}

we then decide that the other end lives in a simplified reset():

void reset() noexcept(/* check that ~T is safe*/) {
  if (_engaged) {
    _value.~T();
    _engaged = false;
  }
}

and now no other code touches _engaged. This fixes a subtle bug in your move ctor, where you set _engaged before you construct the object!

optional(optional&& rhs) noexcept(/* proper condition here*/) {
  if (rhs._engaged) {
    emplace(std::move(*rhs));
  }
}

plus, you don't steal the object, you just move it on move-construct.

optional& operator=(optional&& rhs) noexcept {
  if (*this && rhs) {
    **this = std::move(*rhs);
  } else if (rhs) {
    emplace(std::move(*rhs));
  } else {
    reset();
  }
  return *this;
}

we are now using nice, well documented APIs that are easy to understand to implement a harder function like operator=.

template<typename U>
[[nodiscard]] T value_or(U&& other_value) const & noexcept 
                        requires(std::is_copy_constructible_v<T> && std::is_convertible_v<U&&, T>)
{
  return *this?**this:static_cast<T>(std::forward<U>(other_value));
}

the result is an optional that uses low-level std functions like construct_at fewer times, and accesses _value directly in 5 functions and _engaged in 3.

(You need _engaged in emplace and in reset and in has_value; everything else can call those 3. For _value, you'll need it in emplace, reset, and get methods for &, const& and &&; everything else can call those 5.)

I believe you can do away with all but 1 level of {} indentation in your functions as well, which makes them easier to understand.

After you do all of this, and implement the rest of it (the external API), and unit tests, then you should start with performance testing.

In the code above we have values initialized with one value that are immediately set to another value. The thing is that if the code can be locally understood and the compiler can show that nobody could see the first value, it is capable of skipping that first initialization.

Avoiding stuff like this is often a false optimization; making code more complex to optimize a case that the compiler can handle for you.