debugcppMinor
Class for Value + Error Code
Viewed 0 times
errorvalueforcodeclass
Problem
This is a class similar to
... and here is some usage. I did usage with
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
might do you more harm than good. In particular, if both
Maybe a plain old function would serve you better.
Take sink arguments by-value
It seems to me that you expect
Of course, you could also decide to take both parameters by forwarding reference, just in case.
Consider special-casing the “success” case
Currently, your
For example, you could add a third
Then you could offer a single-argument constructor
and your happy path would be just
Note that I have deliberately not marked the single-argument constructor as
If your
This would allow code like this.
Instead of a single
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
The usual way to handle this is to use a properly aligned buffer instead of a typed variable
In your constructor, you use placement-
Your
Don't forget to invoke the destructor, too.
You would remove the two-argument constructor now. In order to construct an object in an error-state, you'll need a
The
It must never be called without setting
You will also need to write custom copy / move constructors and assignment
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
`
Avoid implicit conversion operators
The implicit conversion
operatortemplate
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.