Does this function have explicit return values on all control paths?

user_8689049 0
Viewed 4175

I have a Heaviside step function centered on unity for any data type, which I've encoded using:

template <typename T>
int h1(const T& t){
   if (t < 1){
       return 0;
   } else if (t >= 1){
       return 1;
   }
}

In code review, my reviewer told me that there is not an explicit return on all control paths. And the compiler does not warn me either. But I don't agree; the conditions are mutually exclusive. How do I deal with this?

return code-coverage templates c++
3 Answers

It depends on how the template is used. For an int, you're fine.

But, if t is an IEEE754 floating point double type with a value set to NaN, neither t < 1 nor t >= 1 are true and so program control reaches the end of the if block! This causes the function to return without an explicit value; the behaviour of which is undefined.

(In a more general case, where T overloads the < and >= operators in such a way as to not cover all possibilities, program control will reach the end of the if block with no explicit return.)

The moral of the story here is to decide on which branch should be the default, and make that one the else case.

user_23808300

Just because code is correct, that doesn't mean it can't be better. Correct execution is the first step in quality, not the last.

if (t < 1) {
    return 0;
} else if (t >= 1){
    return 1;
}

The above is "correct" for any datatype of t than has sane behavior for < and >=. But this:

if (t < 1) {
    return 0;
}
return 1;

Is easier to see by inspection that every case is covered, and avoids the second unneeded comparison altogether (that some compilers might not have optimized out). Code is not only read by compilers, but by humans, including you 10 years from now. Give the humans a break and write more simply for their understanding as well.

user_21924940

As noted, some special numbers can be both < and >=, so your reviewer is simply right.

The question is: what made you want to code it like this in the first place? Why do you even consider making life so hard for yourself and others (the people that need to maintain your code)? Just the fact that you are smart enough to deduce that < and >= should cover all cases doesn't mean that you have to make the code more complex than necessary. What goes for physics goes for code too: make things as simple as possible, but not simpler (I believe Einstein said this).

Think about it. What are you trying to achieve? Must be something like this: 'Return 0 if the input is less than 1, return 1 otherwise.' What you've done is add intelligence by saying ... oh but that means that I return 1 if t is greater or equal 1. This sort of needless 'x implies y' is requiring extra think work on behalf of the maintainer. If you think that is a good thing, I would advise to do a couple of years of code maintenance yourself.

If it were my review, I'd make another remark. If you use an 'if' statement, then you can basically do anything you want in all branches. But in this case, you do not do 'anything'. All you want to do is return 0 or 1 depending on whether t<1 or not. In those cases, I think the '?:' statement is much better and more readable than the if statement. Thus:

return t<1 ? 0 : 1;

I know the ?: operator is forbidden in some companies, and I find that a horrible thing to do. ?: usually matches much better with specifications, and it can make code so much easier to read (if used with care) ...

user_50124210
Related