tags:

views:

2019

answers:

4
$ cat t.cpp
int sign(int i) {
    if(i > 0) return 1;
    if(i == 0) return 0;
    if(i < 0) return -1;
}
$ g++ -c t.cpp -Wall
t.cpp: In function ‘int sign(int)’:
t.cpp:5: warning: control reaches end of non-void function
$

What do I do about this?

Stop using -Wall as it's clearly wrong? Add a bogus return 0 at the end? Clutter the code with "else" clauses?

+11  A: 

If you don't want to add "else" clauses because they would make the code longer, then perhaps you would like to remove the final "if" and make the code shorter:

int sign(int i) {
    if(i > 0) return 1;
    if(i == 0) return 0;    
    return -1; // i<0
}

Or if you're really computing "sign" yourself and this isn't a simplification of some longer example:

int sign(int i) {
    return (i>0) ? 1 : ((i<0)?-1:0);
}
Eric
removing the final check seems alright, thanks
Why shouldn't he compute sign himself? It's not a standard function.
Rob Kennedy
Right you are, Rob; I withdraw my objection.
Eric
+8  A: 

Your sign() function isn't very efficient. Try this

int sign(int i) {
    return (i > 0) - (i < 0);
}

Source: Bit Twiddling Hacks

Christoph
As elegant a solution as this is for computing the sign, would it have been better to answer the question regarding the warning instead?
romandas
+1, for the comment :)
Helltone
@romandas: Solutions for removing the warning were already covered by 2 other answers, so I didn't bother to mention them again; also, when using my solution, the warning will go away as well!
Christoph
The efficiency of the code is surely a trait of the compiler and not the code. Instead of resorting to this hack relying on some weird notion of booleans as integers, I liked the code the clearly expressed its intent. Don't fix compiler bugs with bad code.
John Nilsson
Just like add that if bit twiddling is really what you want, how about doing something with the actual sign bit instead?
John Nilsson
@John: treating integers as booleans might be weird, but it's one of the things you have to get used to when programming in C; and yes, I'd call a one-statement solution without branching more efficient than a 3-statement solution with branching...
Christoph
@John, regarding bit twiddling to read the sign bit: if you want to stay portable, things can get ugly...
Christoph
@John: The function is named "sign". That's a pretty clear expression of intent.
SCFrench
Wow, that just boggled my mind. +1.
Jeff M
Christoph: it is portable.
Nate879
@Nathan: that was my point: this version is, the ones which read the sign bit by shifting aren't if you don't jump through some major hoops
Christoph
I like this code snipped Christoph. The one thing that puzzles me however, is that isn't boolean true defined to be anything != 0? So technically couldn't some compiler end up returning -5 for (i > 0) and still be within the standards?I must be missing something.
Eddie Parker
Ah; got it: From Bjarne's "The C++ Programming Language", section 4.2:"By definition, true has the value 1 when converted to an integer and false has the value 0. Conversely, integers can be implicitly converted to bool values: nonzero integers convert to true and 0 converts to false."
Eddie Parker
+3  A: 

In this case, I'd go for the solution:

int sign(int i)
{
    if (i > 0)
        return 1;
    else if (i == 0)
        return 0;    
    else
        return -1; // i<0
}

That is, I would add two else clauses - to make the code more symmetric, rather than because it makes any difference to the object code generated.

I did some experimentation. I expected the one-line version using the the ternary operator twice to generate the same code as the longer. However, testing on Solaris 10 (SPARC) with GCC v4.3.2 shows that the ternary operator version is consistently 12-16 bytes smaller than the 'if' version. However, the presence or absence of the extra else does make no difference. (Adding register made no odds, as I'd expect.) Added I also looked at Christoph's solution with 'return (i > 0) - (i < 0);' - a variant I'd not seen before. The code sizes were:

       Unoptimized     Optimized (-O5)
 if      166             110
 ?:      150              98
 >-<     122              98

Which mostly goes to show that measurement is a good idea!

Jonathan Leffler
wait what's O5? :)
Hmmm...O5 is a bogus level of optimization. And GCC didn't tell me. Drat! I wonder if I still have the test code around?
Jonathan Leffler
+2  A: 

else clauses are not "clutter", they are a more obvious way of stating your intent.

andy