Avoid returning by-reference argument

Question

Suppose I have a class Option:

template<typename T>
class Option {
public:
    Option() noexcept
    {}

    Option(T val) noexcept : val_(std::make_shared<T>(std::move(val)))
    {}

    const T & get() const
    {
        if (val_ == nullptr) {
            throw std::out_of_range("get on empty Option");
        }
        return *val_;
    }

    const T & getOrElse(const T &x) const
    {
        return val_ == nullptr ? x : *val_;
    }

private:
    std::shared_ptr<T> val_;
};

The argument passed to Option::getOrElse is the default value to return when this Option is empty:

Option<int> x;  // empty
int y = 123;
x.getOrElse(y);  // == 123

However, I think the following code is not safe:

Option<int> x;
x.getOrElse(123);  // reference to temporary variable!

A safer way would be to return by value from Option::getOrElse, but that would be wasteful when the Option is non-empty. Can I work around this somehow?

UPDATE: I'm thinking about perhaps overloading on the argument type (lvalue/rvalue) of getOrElse, but haven't figured out exactly how to do so.

UPDATE 2: Maybe this?

T getOrElse(T &&x) const { ... }

const T & getOrElse(const T &x) const { ... }

But I think this might be ambiguous because both lvalue and rvalue arguments fit the second version.

Answer 1

Can I suggest to re-design this class?

It has a default ctor which can leave the val_ to be nullptr, but it has a get() at the same time which may throw exception because of dereference (*). It also designed to save T in shared_prt but return it as reference.

Let the client to know it's null:

template<typename T>
class Option {
public:
    Option() noexcept
    {}

    Option(T val) noexcept : val_(std::make_shared<T>(std::move(val)))
    {}

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

    bool IsNull() const
    {
        return val_ == nullptr;
    }

private:
    std::shared_ptr<T> val_;
};

The client code changed from:

Option option;
const T & ref = option.getOrElse(123);

to be:

Option option;
const T & ref = option.IsNull() ? 123 : option.get();

Why I delete the: if (val_ == nullptr) {

Let's make make_shared<> clear:

1. return a valid pointer, or
2. throw bad_alloc exception; it does not return null

So IsNull() is also useless, it should be like:

template<typename T>
class Option {
public:
    Option(T val) noexcept : val_(std::make_shared<T>(std::move(val)))
    {}

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

private:
    std::shared_ptr<T> val_;
};

Why to use shared_ptr? option objects can be move or copied several times? or else I prefer to design it like:

template<typename T>
class Option {
public:
    Option(T val) noexcept : val_(std::move(val))
    {}

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

private:
    T val_;
};

Answer 2

I'd rather return by reference and let the caller decide, whether he wants to store a reference to or a copy of the returned value.

Answer 3

Since users of your class can expect the reference returned from Option::get() to be valid only as along as the the particular instance of the Option object's lifetime, you could reasonably make the same expectation for what is returned from Option::getOrElse().

In that case it might be an acceptable overhead for the object to maintain a collection of things that it needs to keep alive for the client:

#include <list>
#include <memory>
#include <iostream>

template<typename T>
class Option {
public:
    Option() noexcept
    {}

    Option(T val) noexcept : val_(std::make_shared<T>(std::move(val)))
    {}

    const T & get() const
    {
        if (val_ == nullptr) {
            throw std::out_of_range("get on empty Option");
        }
        return *val_;
    }

    const T & getOrElse(const T &x) const
    {
        if (val_ == nullptr) {
            std::cout << "storing const T &\n";
            elses_.push_front(x);
            return elses_.front();
        }

        return *val_;
    }

    const T & getOrElse(T &&x) const
    {
        if (val_ == nullptr) {
            std::cout << "storing T && by move\n";
            elses_.push_front(std::move(x));
            return elses_.front();
        }

        return *val_;
    }


private:
    std::shared_ptr<T> val_;
    mutable std::list<T> elses_;
};


int main()
{
    Option<int> x;  // empty
    int y = 123;
    auto rx = x.getOrElse(y);  // == 123

    auto & rxx = x.getOrElse(42); 

    std::cout << "rx = " << rx << "\n";
    std::cout << "rxx = " << rxx << "\n";
}

The references returned by Option::getOrElse() will be valid for as long as the reference returned from Option::get() would be. Of course, this also means that Option::getOrElse() can throw an exception.

As a small improvement, if the T type can be used as keys for an associative container you could use one of those instead of a std::list and easily avoid storing duplicates.

Answer 4

However, I think the following code is not safe:

Option<int> x;
x.getOrElse(123);  // reference to temporary variable!

You are correct. This is why std::optional::value_or() returns a T and not a T& or T const&. As per the rationale in N3672:

It has been argued that the function should return by constant reference rather than value, which would avoid copy overhead in certain situations:

void observe(const X& x);

optional<X> ox { /* ... */ };
observe( ox.value_or(X{args}) );    // unnecessary copy

However, the benefit of the function value_or is only visible when the optional object is provided as a temporary (without the name); otherwise, a ternary operator is equally useful:

optional<X> ox { /* ... */ };
observe(ox ? *ok : X{args});            // no copy

Also, returning by reference would be likely to render a dangling reference, in case the optional object is disengaged, because the second argument is typically a temporary:

optional<X> ox {nullopt};
auto&& x = ox.value_or(X{args});
cout << x;                              // x is dangling!

I suggest you follow the same guidelines. If you really need to avoid the copy, use a ternary. This is safe and copyless:

Optional<int> ox = ...;
const int& val = ox ? *ox : 123;

If you really don't, or the Optional is an rvalue anyway, getOrElse() is more concise.