how to get rid of warning: control reaches end of non-void function c++

Question

I wrote this code and don't understand why there´s a warning, as far as I can tell every branch has a return value. How could I fix this?

bool ValidDate(int d, int m, int a) {
    if ((m < 1) || (m > 12)) {
        return false;
    } else {
        if ((m == 1) || (m == 3) || (m == 5) || (m == 7) || (m == 8) || (m == 10) || (m == 12)) {
            if (d >= 1 && d <= 31) {
                return true;
            } else {
                return false;
            }
        } else if ((m == 4) || (m == 6) || (m == 9) || (m == 11)) {
            if (d >= 1 && d <= 30) {
                return true;
            } else {
                return false;
            }
        } else if (m == 2) {
            if (a % 4 == 0) {
                if (d >= 1 && d <= 29) {
                    return true;
                } else {
                    return false;
                }
            } else {
                if (d >= 1 && d <= 28) {
                    return true;
                } else {
                    return false;
                }
            }
        }
    }
}

I undestand what the error means but can´t find where the problem is.

Answer 1

The warning arises because the compiler does not know that the else if (m == 2) condition will always be true when that condition is checked, as all other possibilities must have been exhausted. Since the compiler does not perform this kind of analysis, you as the programmer might as well help the compiler by explicitly saying so by just removing that check. You can still leave in a comment saying "m must be 2" to help human readers of the code.

The nested if-else branches can also be simplified considerably by noting the following equivalent constructs:

A check like

if (expr)
  return true;
else
 return false;

can always be replaced by

return expr;

Similarly, if you're returning at the end of a branch, the subsequent else is redundant. i.e.

if (expr1) {
  // ...
  return // ...;
}
else if (expr2) {
  // ...
  return // ...;
}

can be replaced by

if (expr1) {
  // ...
  return // ...;
}
if (expr2) {   // the else is redundant here
  // ...
  return // ...;
}

Applying these transformations give a cleaner function:

bool ValidDate2(int d, int m, int a) {
    if ((m < 1) || (m > 12)) 
        return false;
    
    if ((m == 1) || (m == 3) || (m == 5) || (m == 7) || (m == 8) || (m == 10) || (m == 12)) 
        return d >= 1 && d <= 31; 
         
    if ((m == 4) || (m == 6) || (m == 9) || (m == 11)) 
        return d >= 1 && d <= 30;
     
    // Now m must be 2 
    return a % 4 == 0 ?
            d >= 1 && d <= 29 :
            d >= 1 && d <= 28; 
}

Note that I've replaced the last if-else by a ternary operator ?:, though whether this makes the code more readable is subjective. Certainly nesting multiple ?: can make it harder to see the structure of the branches.

Here's a demo.

---

Another way of restructuring the code is to use a switch statement for all the different values of m. This reduces the complexity of the if conditionals, and can make the code easier to read:

bool ValidDate2(int d, int m, int a) {
    
    switch(m) {
        case 1: [[fallthrough]];
        case 3: [[fallthrough]];
        case 5: [[fallthrough]];
        case 7: [[fallthrough]];
        case 8: [[fallthrough]];
        case 10: [[fallthrough]];
        case 12: return d >= 1 && d <= 31; 
        
        case 4: [[fallthrough]];
        case 6: [[fallthrough]];
        case 9: [[fallthrough]];
        case 11: return d >= 1 && d <= 30;

        case 2:  return a % 4 == 0 ?
                    d >= 1 && d <= 29 :
                    d >= 1 && d <= 28;

        default: return false;  // m < 1 or m > 12
    }
}

Here's a demo.