Variant optional to return error string or value

Question

I needed a class with more information than optionals give, I need to know why the optional is not valid in error states. I used a Boost variant for this. Since this will be wrapping almost all of my return values, I need to know if it's as fast as possible (i.e. did I do move semantics and all that jazz properly?).

template<typename T>
class Result
{
public:
    Result(const std::string& errorString) {
        contents = errorString;
    }

    Result(T&& value) {
        contents = std::forward<T>(value);
    }

    operator bool() {
        //is the active value our T
        return contents.which() == 0;
    }

    T unwrap() {
        return boost::get<T>(contents);
    }

    std::string unwrapErr() {
        return boost::get<std::string>(contents);
    }

private:
    boost::variant<T, std::string> contents;
};

Example use

Result<std::vector<int>> foo()
{
    if(success)
         return Result<std::vector<int>>({1,2,3});
    else
         return Result<std::vector<int>>("failure reason");
}

if(auto vec = foo())
   vec.unwrap() //do stuff to contained value
else
   std::cout << vec.unwrapErr();

Answer 1

You said:

I need to know if it's as fast as possible

The only thing I could think of that would make your program potentially faster is:

Initialize the member variable using initialization list syntax

Result(const std::string& errorString) : contents(errorString) {}

// boost::variant already has a constructor that takes a rvalue reference.
Result(T&& value) : conents(value) {}

Other suggestions:

Add operator T functions instead of unwrap

This may be a matter of stylistic preference but I think it is better to use:

Result<int> r = <some function call>;
if ( r && r == 10 )
{
    // Do something with r
}

than

Result<int> r = <some function call>;
if ( r && r.unwrap() == 10 )
{
    // Do something with r
}

You can use the former syntactic construct by providing const and non-const versions of user defined conversion operators.

operator T const& () const
{
   return boost::get<T>(contents);
}
operator T& ()
{
   return boost::get<T>(contents);
}

Decide how to deal with use of Result<std::string>

If you use Result<std::string>, the function operator bool will always return true. For that use case, there is no way to tell whether a function returned with success or failure.

One way to deal with the problem will be to prevent use of Result<std::string> using static_assert.

static_assert(std::is_same_type<T, std::string>::value == false,
              "Can't use std::string as template paramete");

Use a better name than unwrapErr()

I would prefer to use something like getError() or getErrorString().

Update

The operator T functions will be problem when T is bool. If you like to use the operator T functions for all types, it will be better to change operator bool to a different function, such as isGood().

bool isGood() {
    //is the active value our T
    return contents.which() == 0;
}

then, you'll have to use

Result<int> r = <some function call>;
if ( r.isGood() && r == 10 )
{
    // Do something with r
}

Answer 2

I have added two suggestions.

1. Create a wrapper for errors, so you can different it from std::string and you will be able to use Result<std::string> too.

   struct Err {
     std::string msg;
   };
   
   

2. Use the dereference operator to access the content of the Result class, as C++ std classes do. It will make the Result class work with bool too.

   const T& operator *() const {
       return std::get<T>(contents);
     }
   
     T& operator *() {
       return std::get<T>(contents);
     }
   

   So you can use this way:

   Result<Foo> result = doSomething();
   Foo resultValue = *result;
   

   You can even add the pointer operator too!

     const T* operator ->() const {
       return &std::get<T>(contents);
     }
   
     T* operator ->() {
       return &std::get<T>(contents);
     }
   

Final code and some usages:

struct Err {
  explicit Err(std::string s) : msg(std::move(s)) {}
  std::string msg;
};

template<typename T>
class Result
{
public:
  Result(const Err& errorString) : contents(errorString) {
  }

  Result(T&& value) : contents(value) {
  }

  Result<std::string>(const char* v) : contents(v) {} 

  explicit operator bool() const {
    return contents.index() == 0;
  }

  const T& operator *() const {
    return std::get<T>(contents);
  }

  T& operator *() {
    return std::get<T>(contents);
  }

  std::string getErrorMessage() {
    return std::get<Err>(contents).msg;
  }

private:
  std::variant<T, Err> contents;
};

Usage:

  BOOST_AUTO_TEST_CASE(TestResultClass)
  {
    Result<int> r1(1);
    BOOST_REQUIRE(r1);
    BOOST_REQUIRE(*r1 == 1);
    Result<int> r2(Err{"Erro"});
    BOOST_REQUIRE(!r2);
    BOOST_REQUIRE(r2.getErrorMessage() == "Erro");
    Result<bool> r3(false);
    BOOST_REQUIRE(r3);
    BOOST_REQUIRE(*r3 == false);
    Result<bool> r4(Err{"Erro"});
    BOOST_REQUIRE(!r4);
    BOOST_REQUIRE(r4.getErrorMessage() == "Erro");
    Result<bool> r5(true);
    BOOST_REQUIRE(r5);
    BOOST_REQUIRE(*r5 == true);

    Result<std::string> r6("Sucesso");
    BOOST_REQUIRE(r6);
    BOOST_REQUIRE(*r6 == "Sucesso");
    Result<std::string> r7(Err{"Erro"});
    BOOST_REQUIRE(!r7);
    BOOST_REQUIRE(r7.getErrorMessage() == "Erro");

  }

And:

Result<bool> foo() {
   if (a-error) {
      return Err{"My error message"};
   }
   
   return bar == 100; //not error return as boolean
}

Edit: I have added the explicit keyword for the Err class, pointed by @G. Sliepen. And I added a specialization to create Result<std::string> with const char*, so we can just return const char* from functions with Result<std::string> return type.

Answer 3

This obviously isn't suitable for functions that return std::string. We might be able to address that if we access values by index rather than by type.

You should be able to use std::variant for this, and not depend on Boost.

std::forward() is for forwarding references. But T&& here is an rvalue reference. So we're unable to construct from an lvalue:

std::vector v = {1,2,3};
return Result<std::vector<int>>{v}; // compile error

We should be accepting by value, and using std::move to minimise copying:

    Result(std::string errorString)
        : contents{std::move(errorString)}
    {}

    Result(T value)
        : contents{std::move(value)}
    {}

It's probably worth a comment to mention that implicit conversions are intentionally allowed here, as we'd normally expect single-argument constructors to be explicit.

The member functions ought to be const, as they do not modify the value. And the "unwrap" functions can return reference to const, which avoids unnecessary copying:

    // True if it contains a T
    operator bool() const;

    const T& unwrap() const;

    const std::string& unwrapErr() const;

---

Suggested improved code:

#include <string>
#include <vector>
#include <variant>

template<typename T>
class Result
{
    std::variant<std::string, T> contents;

public:
    Result(std::string errorString)
        : contents{std::move(errorString)}
    {}

    Result(T value)
        : contents{std::move(value)}
    {}

    // True if it contains a T
    explicit operator bool() const {
        return contents.index();
    }

    const T& unwrap() const {
        return std::get<1>(contents);
    }

    const std::string& unwrapErr() const {
        return std::get<0>(contents);
    }
};

---

I moved the error string to be the first element of the variant, as this might be a step towards instantiating with more than one type argument (template<typename... T>), to avoid wrapping a variant return inside the variant.