0

Sorry in advance if the question sounds naive. I am writing a simple bool function to check if all digits of a number are same. The following works, however the one after, gives me an error of

17:1: warning: control reaches end of non-void function [-Wreturn-type] 

What I am doing wrong with the second one?

Working:

# include <iostream> # include <string> using namespace std; bool samedigits (int number){ bool result = true; std::string snumber = to_string (number); for (int i=0; i < snumber.size(); i++){ if (snumber[i] != snumber[0]){ result = result and false; break; } } return result; } int main() { cout << samedigits (666); return 0; } 

Non working:

# include <iostream> # include <string> using namespace std; bool samedigits (int number){ std::string snumber = to_string (number); for (int i=0; i < snumber.size(); i++){ if (snumber[i] != snumber[0]){ return false; break; } return true; } } int main() { cout << samedigits (666); return 0; } 
7
  • 3
    If snumber is empty, how many times does for (int i=0; i < snumber.size(); i++) run? If the loop never runs, does the function ever reach a return statement? Also think on what use is a for loop that returns on ever iteration of the loop. Commented Sep 28, 2020 at 4:16
  • 1
    The codes are not equivalent. The loop in the second snippet is useless. Commented Sep 28, 2020 at 4:21
  • The second code snippet will always return in the first loop iteration (and since snumber[0] is equal to itself, will always return true. The warning is a bit spurious, but compilers tend to get false positives in identifying non-reachable code when the structure is a bit convoluted as your is. In the first result = result and false has the same effect as result = false. Commented Sep 28, 2020 at 4:39
  • Are you aware that the break in the second example is useless (dead code)? Commented Sep 28, 2020 at 4:45
  • @JaMiT I do not think it is totally useless as it saves time. Imagine the string is too long, the first instance it finds a different digit it stops the loop Commented Sep 28, 2020 at 5:58

3 Answers 3

2

Your algorithm is incorrect, you are only checking if the first character is equal to itself, and returning true for every input. Instead, you should move the return true outside the loop, so that you check all the characters first.


Unfortunately, the warning is a false positive. The compiler fails to realize that std::to_string is guaranteed to return a non-empty string when passed an int. This means that the for loop body will be entered, and the function will return a value.

Sign up to request clarification or add additional context in comments.

4 Comments

If I move the return true out of the loop then it will always return true when the for loop is finished. Am I missing something?
What can you say about the string if the for loop finishes without hitting the if condition?
I think I would not call the warning a "false positive" since your analysis relies on information outside the function. I'd say it is a true positive, but can be proven irrelevant. I don't know if there is a good term for that, though. :( Maybe "technical positive"? "Irrelevant positive"? Meh, I guess leave it for now, and consider this comment an invitation for people to think of a better term.
@JaMiT Hmm, I think "false positive" is the right term. Not that I'm suggesting the compiler should ever be expected to get this right, but the warning is still wrong.
0

The compiler is right. There is a code path in your second snippet that won't return.

for (int i=0; i < snumber.size(); i++){ 

Here, the std::string size function can return 0 according to its contract. When it does happen, then your function won't enter the loop. After that, you exit the function without returning.

1 Comment

Can't happen, but the compiler can't see that.
0

The second version of your function (combined with some information obtained via comments) indicates a misunderstanding of what return does. The second version would work (here is the misunderstanding) if a return statement were to simply store the indicated value for use when the function eventually ends. However, this is not how return works. A return statement stores the indicated value for use when the function ends and immediately ends the function. In the second version of your function, the for statement might as well be an if and the break is never executed as it comes right after a return.


To demonstrate, let's do a code walk-through for a call to samedigits(123).

bool samedigits (int number){ 

As we enter the function, number is set to 123.

std::string snumber = to_string (number); 

The string snumber is set to "123".

for (int i=0; i < snumber.size(); i++){ 

The loop initializes by setting i to 0 then checks if 0 < 3 (the size of snumber is 3). This is true, so we enter the loop. Note that the result of entering the loop depends only on snumber not being empty.

 if (snumber[i] != snumber[0]){ 

We check to see if snumber[0] does not equal snumber[0]. This is a bit trivial, but the computer is willing to do it. The result, of course, is false (independent of what the input was – this part might be more interesting if the loop started at 1 instead of 0). So skip to the statement after the if.

 return true; 

The function immediately ends, returning true.

And that's it. There is no second iteration of the loop. No other code is executed during this function call. Since to_string never returns an empty string, the second version of your function is functionally equivalent to the following:

bool samedigits (int /*number*/){ return true; // Execution ends with the return statement, so nothing after // this comment ever executes. std::cout << "This is dead code. It will never execute.\n"; std::cout << "You will never see this output.\n"; } 

To fix the second version, you need to return inside the loop only when the if condition is true. Move return true; to be after the loop so that the loop can iterate multiple times. You do not want to end the function and tell the caller that all the digits are the same (which is what return true; does) until after you've checked all the digits. (If your loop finds a mismatch, execution will reach the return false; inside the loop. At that point, the function ends, so code after the loop has no effect on that function call.)

A smaller (cosmetic) fix is to get rid of the break. Suppose the loop did iterate enough times to find a mismatch. Execution would go into the if statement body and encounter return false. At that point, not only is the loop stopped, but the function as a whole ends, before the break statement is seen. The break statement is dead code, meaning code that can never be executed. In order to get to the break, execution has to go through a return. Execution may arrive at a return statement, but it never goes through one.

Also, be sure to thank your compiler for finding this error, as it does point to a bug in your code. It's not the exact bug the compiler reported, but then again, compilers are not exactly the best thinkers in the world. :)

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.