HiveBrain v1.2.0
Get Started
← Back to all entries
debugcppMinor

Class for Value + Error Code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
errorvalueforcodeclass

Problem

This is a class similar to std::pair and std::tuple inspired by Alexandresku's Expected. Let me know if this class can be helpful for simple error handling.

template
class CheckValue{
public:
    template
    CheckValue(const E &e, UT &&t) : e(e), t(std::forward(t)){}

    operator E() const{
        return e;
    }

    T &get(){
        return t;
    }

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

private:
    E e;
    T t;
};


... and here is some usage. I did usage with bool first, but deliberately redesign it to work with enum class to show full power.

enum class CalcError{
    OK,
    ERROR,
    DIV_BY_ZERO
};

CheckValue calc(float a, float b){
    if (b == 0)
        return { CalcError::DIV_BY_ZERO, 0 };

    return { CalcError::OK, a / b };
}

void prn(float a, float b){ 
    switch( auto val = calc(a, b) ){
    case CalcError::DIV_BY_ZERO:
        std::cout << "Division by zero" << std::endl;
        break;

    case CalcError::ERROR:
        std::cout << "Error"        << std::endl;
        break;

    default:
    case CalcError::OK:
        std::cout << val.get()      << std::endl;
        break;
    }
}

int main(){
    prn(5, 2);
    prn(5, 0);
}

Solution

The code looks quite good to me in general. Here are a few remarks.

Avoid implicit conversion operators

The implicit conversion operator

template 
CheckValue::operator E() const;


might do you more harm than good. In particular, if both E and T are of arithmetic type, forgetting to call get will happily use the error code instead of the value and you'll probably have a bad time tracing down this bug.

Maybe a plain old function would serve you better.

template 
E
CheckValue::status() const;


Take sink arguments by-value

It seems to me that you expect E to be some cheaply copyable value type, like an enum or an integer so you can easily afford always making a copy. That's perfectly reasonable but then I'm surprised why you take it by const reference. Just take it by-value instead. If you're not so sure whether it will always be cheap to copy, apply std::move to it instead. It should never cost you anything and might sometimes benefit you.

template 
CheckValue(E, UT&& t) : e(std::move(e)), t(std::forward(t)) {}


Of course, you could also decide to take both parameters by forwarding reference, just in case.

template 
CheckValue(UE&& e, UT&& t) : e(std::forward(e)), t(std::forward(t)) {}


Consider special-casing the “success” case

Currently, your class does not know any semantics about E. If its intention is to be used as “success or failure” indicator, you could make this explicit. This would also allow you to omit the error code when there actually is no error.

For example, you could add a third template parameter that indicates the “success” value (note that I have re-ordered the parameters).

template (0)>
class CheckValue;


Then you could offer a single-argument constructor

template 
template 
CheckValue::CheckValue(V&& v)
    : e(Good), t(std::forward(v)) {}


and your happy path would be just return 42;.

Note that I have deliberately not marked the single-argument constructor as explicit in this case. Otherwise, you'd have to write return {42};. Which version you prefer is probably a matter of taste.

If your class knows about the distinct “success” case, you can add a check for it.

template 
explicit
CheckValue::operator bool() const noexcept
{
  return (this->e == Good);
}


This would allow code like this.

if (const auto cv = prn(5, 2))
    std::cout << cv.get() << '\n';
else
    std::cerr << "error: " << to_string(cv.status()) << '\n';


Instead of a single Good value, you could have a more general predicate but this would deprive you of the ability to default it in the lucky case and smells like over-engineering to me.

Think whether you want a “no value” state

If your computation fails, you currently have to pass a dummy value to the constructor because it always initializes the t member. If an object of type T is expensive to construct, this would be wasteful. It also feels bad having to specify a value when you actually have none. Of course, you also don't want to default-construct the t member in this case as this might be equally costly or not even possible.

The usual way to handle this is to use a properly aligned buffer instead of a typed variable T.

char buffer[sizeof(ValueT)] alignas(ValueT);


In your constructor, you use placement-new to create an object inside the buffer, if you have one.

template 
template 
CheckValue::CheckValue(V&& v) : e(Good)
{
  new (this->buffer) ValueT(std::forward(v));
}


Your get function would then cast the buffer to a pointer of the appropriate type, after optionally checking that there is a value in it.

template 
ValueT&
CheckValue::get()
{
  assert(this->e == Good);
  return *reinterpret_cast(this->buffer);
}


Don't forget to invoke the destructor, too.

template 
CheckValue::~CheckValue()
{
  if (this->e == Good)
    reinterpret_cast(this->buffer)->~ValueT();
}


You would remove the two-argument constructor now. In order to construct an object in an error-state, you'll need a static factory function.

template 
CheckValue
CheckValue::make_failure(StatusT e)
{
  assert(e != Good);
  CheckValue bad {};  // use private default-constructor
  bad.e = e;
  return bad;
}


The private default constructor does not initialize the object.

template 
CheckValue::CheckValue() noexcept {}


It must never be called without setting e to anything but Good immediately afterwards. (Or the destructor and the other functions would invoke undefined behavior.) Therefore, it is essential that this dangerous constructor is private.

You will also need to write custom copy / move constructors and assignment operators. You might want to implement them in terms of a custom swap overload.

Consider restricting the type of the error code

Since the code makes a few assumptions about the type of the error code, you might as well make these explicit and restrict it to POD types. To do this, simply put

`

Code Snippets

template <class E, class T>
CheckValue<E, T>::operator E() const;
template <class E, class T>
E
CheckValue<E, T>::status() const;
template <class UT>
CheckValue(E, UT&& t) : e(std::move(e)), t(std::forward<UT>(t)) {}
template <class UE, class UT>
CheckValue(UE&& e, UT&& t) : e(std::forward<UE>(e)), t(std::forward<UT>(t)) {}
template <class ValueT, class StatusT, StatusT Good = static_cast<StatusT>(0)>
class CheckValue;

Context

StackExchange Code Review Q#123892, answer score: 2

Revisions (0)

No revisions yet.