5
\$\begingroup\$

I decided to make my own version of C++17's std::optional

It's basically a class that can optionally hold a variable (as the name suggests), it avoids having to resort to std::pair<bool, T> to check whether a variable is set.

I'd like some feedback regarding the overall implementation

template<typename T>
class Optional {
 public:
  /**
   * The constructor
   */
  Optional() :
    is_set_(false) {}
  /**
   * The constructor
   * @param val The value to set it to
   */
  Optional(const T& val) :
    val_(val), is_set_(true) {}

  /**
   * Sets the value
   * @param val The value to set it to
   */
  void set(const T& val) { val_ = val; is_set_ = true; }

  /**
   * @return The value
   */
  T get() const { return val_; }

  /**
   * Clears the value
   */
  void clear() { val_ = T{}; is_set_ = false; }

  /**
   * @return Whether the value is set
   */
  bool empty() { return !is_set_; }

  /* Operators */

  Optional operator=(const T& val) {
    val_ = val;
    is_set_ = true;
    return *this;
  }

  operator T() const {
    return val_;
  }

  bool operator==(const bool& val) const {
    return (is_set_ == val);
  }

  explicit operator bool() const { return is_set_; }

 private:
  T val_{};
  bool is_set_ = false;
};
\$\endgroup\$
  • 2
    \$\begingroup\$ This isn't nearly what std::optional does. Your implementation only works for default constructible types T. This also initialises Twhenever an optional is instantiated. The trick is to use a raw memory buffer with a placement-new. And your get() function should at least return a const reference. If you add some std::move-construction support it looks good otherwise! Keep it on! \$\endgroup\$ – Maikel Mar 20 '17 at 16:20
  • \$\begingroup\$ I don't understand the semantics of your equality comparison operator: return (is_set_ == val); doesn't make much sense for me. If both are set, shouldn't that operation test for the equality of val_ == val.val_? Can you elaborate about that please? In general I have to agree with @Maikel's comment. \$\endgroup\$ – πάντα ῥεῖ Mar 20 '17 at 17:13
  • \$\begingroup\$ @πάνταῥεῖ The boolean operator is to check whether it has been set, not about the actual value. if (optional )/if (optional == true) would be it's usage. If you want to check the actual values you could do if (optional.get() == other_val) \$\endgroup\$ – Lunatoid Mar 20 '17 at 17:21
  • \$\begingroup\$ @Lunatoid As mentioned that semantic is weird and unexpected, you already have an explicit type conversion operator to bool. At least with a Optional<bool> that would get extremely confusing. \$\endgroup\$ – πάντα ῥεῖ Mar 20 '17 at 17:25
  • \$\begingroup\$ @Lunatoid See also here please. \$\endgroup\$ – πάντα ῥεῖ Mar 20 '17 at 17:32
6
\$\begingroup\$

Prefer value type semantic

Try to make classes default constructible and equality comparable whenever this makes sense. Currently your Optional class doesn't fulfil neither. The following code will not compile

class A { int a; A() = delete; };

int main()
{
  Optional<A> i;
}

Even though it might make sense to have an optional. Also consider this example

struct B { B() { /* insert something super long */ } }:

int main()
{
  Optional<B> opt_b; // this line takes really long!
}

Here is an example of how to use std::aligned_storage to use uninitialised memory. I think this could help you as well.

Do not return by value if you can

The line

T get() const { return val_; }

Makes a copy of T while returning val_. Imagine val_ is a very big std::string. This could be very expensive in code which calls get() multiple times. If you want read-only access, prefer to use

const T& get() const& noexcept { return val_; }

instead.

Make use of move assignments

Currently you have to copy values into your optional. But what, if you just want to move something big that you want to return from a function? You need to add constructors for rvalue-Ts.

Optional(T&& value)
 : val_{std::move(value)}
 , is_set_{true} {}
\$\endgroup\$
  • \$\begingroup\$ A few questions arose. In the function declaration of get you changed const to const&, does that do anything significant? And what is the use for noexcept? Why would it throw any exceptions and if so, why would I want to ignore them? \$\endgroup\$ – Lunatoid Mar 20 '17 at 17:10
  • \$\begingroup\$ @Lunatoid They are called ref-qualifiers for member functions akrzemi1.wordpress.com/2014/06/02/ref-qualifiers and open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2439.htm and en.cppreference.com/w/cpp/language/member_functions \$\endgroup\$ – Maikel Mar 20 '17 at 17:22
  • \$\begingroup\$ @Lunatoid noexcept is a contract with the compiler. It is used to tell it, that there won't be a exception and it does not have to prepare any stack unrolling. Since we only return a reference and do not copy anything it is unconditionally true, that no exception is going to happen. Like ref-qualifiers, noexcept is a C++11 feature too. \$\endgroup\$ – Maikel Mar 20 '17 at 17:24
  • \$\begingroup\$ const T& get() const& noexcept means essentially: this is only usable in lvalue expressions and never throws (if it does, the program will terminate). \$\endgroup\$ – Maikel Mar 20 '17 at 17:26
  • \$\begingroup\$ @Maikel I believe there are many more points to discuss about. For me it looks the OP completely screwed that at the semantical level. Nevertheless I upvoted your answer, for pointing out some of the basic flaws. \$\endgroup\$ – πάντα ῥεῖ Mar 20 '17 at 17:35
2
\$\begingroup\$

Maikel covered some issues, but since title says "reinventing std::optional", I'll assume that you want full blown std::optional (which is quite hard to get right, but not as hard as std::variant). I want to add that your code as it stands probably is good for your needs already. I'll also try to answer some of the questions you mentioned in the comments.

Container:

Currently value is stored in the variable of its own type. To make it more robust (such as handling non default constructible types, or types that are expensive to construct), it is possible to store the value in the byte array. Yes, it might sound scary, but it is the easiest approach in my opinion.

using byte = unsigned char;
alignas(T) byte[sizeof(T)] container;

Then it is possible to use placement new to construct the value. Now we control when object will be constructed.

To access the value already stored in it, reinterpret_cast<> is needed. I know this starts sounding dangerous, but it is the way it should be.

T& get() noexcept
{
    return *reinterpret_cast<T*>(&container[0]);
}

const T& get() const noexcept
{
    return *reinterpret_cast<T*>(&container[0]);
}

The destructor will need to be changed then:

~Optional()
{
    reinterpret_cast<T*>(&container[0])->~T();
}

noexcept

noexcept means that the function promises that it won't throw, and if it will, std::terminate is called. It is good because people will know that they can call the function without worrying that it will throw. For example, they could use it in destructors, exception handlers, where throwing again would call std::terminate.

As a side note, if there is any member variable that has throwing destructor, the destructor of the enclosing class will be marked as noexcept(false) (link, see the explanation part), which might get into trouble in generic code.

emplace construction:

With current interface users will have to construct the value and then copy it into the optional, which is wasteful. One way is to create move constructor, the other way is to create conversion constructor (there is no real agreement on how those are called).

template <typename ... ArgTypes>
Optional(std::in_place_t<T>, ArgTypes ... args)
{
    new (&container[0]) T(std::forward<ArgTypes>(args)...);  
}

Notice how I used std::in_place_t<T>. It is there because compiler will treat every call to constructor as conversion constructor. The reason is that ArgTypes... can be anything, which includes bool, int, SomeTypeThatHasNothingToDoAboutIt and Optional<T>&. std::in_place_t<T> will disambiguate that. As an example:

Optional<int> opt(2);
Optional<int> anotherOpt(opt); 
//                       ^^ ArgTypes is T&, not const T&,
//                        so conversion constructor will be called
//                        which is plain wrong.

The way to work around that would be:

Optional<int> anotherOpt(std::as_const(opt));

But it is almost the same typing as std::in_place_t<T>.

assignment:

The class would benefit from implementing operator=(...). Move assigning would also be great. Additionally, there is no way to assign one Optional to another Optional, which would be great to have.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.