5

I'm devoloping an application in LINUX with an older gcc version (7.something if I remember correctly). Recently I tried to run the same application on Windows. On Windows, I'm using MinGW as a compiler (with gcc 8.1.0) .

I came across this error message while compiling my application on Windows:

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

the code is similar to the following:

class myClass {
protected:
    enum class myEnum{
        a,
        b,
    };

    int fun(myClass::myEnum e);
}

and

int myClass::fun(myClass::myEnum e) {
    switch (e){
        case myEnum::a:{
            return 0;
        }
        case myEnum::b:{
            return 1;
        }
    }
}

I understand what the error message means, I'm just wondering why it was never an issue in LINUX.

Is this piece of code really an issue and do I have to add some dummy return statements?

Is there a branch of this function that will lead to no return statement?

user7431005
  • 3,899
  • 4
  • 22
  • 49
  • 3
    Why not just add `default: std::terminate();` or something like that? If your hypotheses that the `default:` case can't be reached, no more warning and no harm done. And if you ever turn out to be incorrect (someone extended `myEnum` or improperly cast to one?) you just caught a nasty error that would have been UB. – François Andrieux Aug 30 '18 at 19:30
  • _"...never an issue in LINUX..."_: You need to increase your warnings level, live: https://godbolt.org/z/JHGQd- – Richard Critten Aug 30 '18 at 19:33
  • 2
    Compilers can issue whatever `warning` messages they like. And yes, I would say that code is an issue - what happens if you update the enum values, but don't update the function? –  Aug 30 '18 at 19:33
  • @FrançoisAndrieux Adding a default clause is bad because then you won't get warnings about unhandled cases if you extend the enum later. – melpomene Aug 30 '18 at 19:33
  • 1
    To answer the question - gcc 8.1.0 must be smarter to catch such issues. Since your function is expected to return something, it should return a value before reaching end of function. – Aditya Bhave Aug 30 '18 at 19:34
  • You code has undefined behavior. – user7860670 Aug 30 '18 at 19:35
  • @melpomene Not all platforms provide that warning anyway. And the question implies a platform where you already get a warning for *not* having a default case. – François Andrieux Aug 30 '18 at 19:35
  • @NeilButterworth in this case compiler will issue another warning, "not all enum values are handled in switch" – SergeyA Aug 30 '18 at 19:35
  • @VTT can you please clarify? I see no undefined behavior. – SergeyA Aug 30 '18 at 19:36
  • 4
    @NeilButterworth "what happens if you update the enum values, but don't update the function? " hmm in that case source must be recompiled and then compiler can issue that warning – Slava Aug 30 '18 at 19:37
  • 2
    Consider `fun(static_cast(2))`. – melpomene Aug 30 '18 at 19:37
  • 1
    @melpomene thats UB and should not be considered by compiler – Slava Aug 30 '18 at 19:38
  • @SergeyA **9.6.3 The return statement [stmt.return]** *"Flowing off the end of a constructor, a destructor, or a function with a cv void return type is equivalent to a return with no operand. Otherwise, flowing off the end of a function other than main (6.6.1) results in undefined behavior."* Also note that enum variable can contain values other than `a` or `b` – user7860670 Aug 30 '18 at 19:38
  • @AdityaBhave no gcc 8.10 is not smarter in this case - logically there is no way that execution would pass that switch statement – Slava Aug 30 '18 at 19:39
  • 3
    @Slava It's not necessarily UB : https://stackoverflow.com/questions/18195312/what-happens-if-you-static-cast-invalid-value-to-enum-class – François Andrieux Aug 30 '18 at 19:39
  • 1
    [Adding __builtin_unreachable fixes it for gcc](https://godbolt.org/z/aGKriN) – Shafik Yaghmour Aug 30 '18 at 19:40
  • @AdityaBhave I can put the g++ version back to 4.7.1 and get the warning, live: https://godbolt.org/z/Cmbxjg – Richard Critten Aug 30 '18 at 19:40
  • @FrançoisAndrieux it would be UB for the case of `fun(static_cast(2))` – SergeyA Aug 30 '18 at 19:45
  • @VTT so what? There is no such case here, as enum can not contain other values unless it was UB during conversion – SergeyA Aug 30 '18 at 19:46
  • 1
    @SergeyA An `enum` can actually have a value other than the those defined by the `enum` without UB. See https://stackoverflow.com/questions/18195312/what-happens-if-you-static-cast-invalid-value-to-enum-class – François Andrieux Aug 30 '18 at 19:59
  • Missing closing semicolon: `class myClass { ... };` It probably should have failed to compile. – jww Aug 30 '18 at 20:07
  • @FrançoisAndrieux hmmm... I am honestly not sure if I am convinced. Defect 1766 states that `The value is unchanged if the original value is within the range of the enumeration values (10.2 [dcl.enum]). Otherwise, the behavior is undefined` [dcl.enum].10 gives following example: `The possible values of an object of type color are red, yellow, green, blue;` - explicitly listing only enum values. Yes, it also says that `For enumeration ... type is fixed, the values of the enumeration are the values of the underlying type. ` - but it doesn't mean that ALL values of this type are valid – SergeyA Aug 30 '18 at 20:30

4 Answers4

4

This is a shortcoming of g++ static analyzer. It doesn't get the fact that all enum values are handled in the switch statement correctly.

You can notice here https://godbolt.org/z/LQnBNi that clang doesn't issue any warning for the code in it's current shape, and issues two warnings ("not all enum values are handled in switch" and "controls reach the end on non-void function") when another value is added to the enum.

Keep in mind, that compiler diagnostic is not standardized in any way - compiler are free to report warnings for conforming code, and report warnings (and compile!) for a malformed program.

SergeyA
  • 61,605
  • 5
  • 78
  • 137
  • Should it be "report no warnings ..." ? You missed no – Slava Aug 30 '18 at 19:43
  • @Slava, nope - I meant exactly that, compilers are allowed to report warnings for conforming program, this is not prohibited by standard. – SergeyA Aug 30 '18 at 19:44
  • So the takeaway message is that my code should be fine, as long as I do not add new elements to the enum? – user7431005 Aug 30 '18 at 19:51
  • 2
    @user7431005 yes, and if you add new values to the enum, you will have another warning stating that not all enum values were handled in the `switch`. You can use `__builtin_unreachable()` as in the other answer to silence it. – SergeyA Aug 30 '18 at 19:52
  • 1
    I actually find that warning "not all enum values handled by switch" even more annoying and useless, because quite often I do it on purpose. So it is strange gcc can detect that whether all enum values handled to produce this useless warning but cannot use that to suppress unreachable return one. – Slava Aug 30 '18 at 19:54
  • @Slava I have a mixed feeling about this warning. Sometimes it is annoying, because not handling all the items is designed, but sometimes it is a savior - when you add a value but didn't update all switches for it. In the case when values are omitted intentionally, it makes sense to just list them as unused lables with break - this will prevent the warning, and unlike using default, will guarantee that added values will have a chance to be reviewed – SergeyA Aug 30 '18 at 19:57
  • The problem here is that you rely on reachability analysis performed by compiler. Which is (a) is not mandatory (b) may yield different results on different compilers or even on same compiler with different settings (c) [may play tricks on you](https://wandbox.org/permlink/TBYQ1BatcEB5G1AH) – user7860670 Aug 30 '18 at 21:37
4

You have to keep in mind that in C++ enums are not what they are appear to be. They are just ints with some constraints, and can easily assume other values than these indicated. Consider this example:

#include <iostream>

enum class MyEnum {
  A = 1,
  B = 2
};

int main() {
  MyEnum m {}; // initialized to 0
  switch(m) {
    case MyEnum::A: return 0;
    case MyEnum::B: return 0;
  }
  std::cout << "skipped all cases!" << std::endl; 
}

The way around this is to either put a default case with assert(false) as VTT indicated above, or (if you can give everybody the guarantee that no values from outside the indicated set will ever get there) use compiler-specific hint, like __builtin_unreachable() on GCC and clang:

  switch(m) {
    case MyEnum::A: return 0;
    case MyEnum::B: return 0;
    default: __builtin_unreachable();
  }
Andrzej
  • 5,027
  • 27
  • 36
1

Firstly, what you describe is a warning, not an error message. Compilers are not required to issue such warnings, and are still permitted to successfully compile your code - as it is technically valid.

Practically, most modern compilers CAN issue such warnings, but in their default configuration they do not. With gcc, the compiler can be optionally configured to issue such warnings (e.g. using suitable command line options).

The only reason this was "never an issue" under linux is because your chosen compiler was not configured (or used with a suitable command line option) to issue the warning.

Most compilers do extensive analysis of the code, either directly (during parsing the source code) or by analysis of some internal representation of that code. The analysis is needed to determine if the code has diagnosable errors, to work out how to optimise for performance.

Because of such analysis, most compilers can and do detect situations that may be problematical, even if the code does not have diagnosable errors (i.e. it is "correct enough" that the C++ standard does not require a diagnostic).

In this case, there are a number of distinct conclusions that the compiler may reach, depending on how it conducts analysis.

  • There is a switch. In principle, code after a switch statement may be executed.
  • The code after the switch reaches the end of the function without a return, and the function returns a value. The result of this is potential undefined behaviour.

If the compiler's analysis gets this far (and the compiler is configured to warn on such things) criteria for issuing a warning are met. Further analysis is then needed if the warning can be suppressed e.g. determine that all possible values of e are represented by a case, and that all cases have a return statement. The thing is, a compiler vendor may elect not to do such analysis, and therefore not suppress warnings, for all sorts of reasons.

  • Doing more analysis increases compilation times. Vendors compete on claims of their compiler being faster, among other things, so NOT doing some analysis is therefore beneficial in getting lower compilation times;
  • The compiler vendor may consider it is better to flag potential problems, even if the code is actually correct. Given a choice between giving extraneous warnings, or not warning about some things, the vendor may prefer to give extraneous warnings.

In either of these cases, analysis to determine that the warning can be suppressed will not be done, so the warning will not be suppressed. The compiler will simply not have done enough analysis to determine that all paths of execution through the function encounter a return statement.

In the end, you need to treat compiler warnings as sign of potential problems, and then make a sensible decision about whether the potential problem is worth bothering about. Your options from here include suppressing the warning (e.g. using a command line option that causes the warning to be suppressed), modifying the code to prevent the warning (e.g. adding a return after the switch and/or default case in the switch that returns).

Peter
  • 35,646
  • 4
  • 32
  • 74
0

One should be very careful when omitting return statements. It is an undefined behavior:

9.6.3 The return statement [stmt.return]

Flowing off the end of a constructor, a destructor, or a function with a cv void return type is equivalent to a return with no operand. Otherwise, flowing off the end of a function other than main (6.6.1) results in undefined behavior.

It may be tempting to consider that this code is fine because all the valid enumerator values (in this case in range 0..1 [0..(2 ^ M - 1)] with M = 1) are handled in switch however compiler is not required to perform any particular reachability analysis to figure this out prior to jumping into UB zone.

Moreover, example from SergeyA's answer shows that this kind of code is a straight time bomb:

class myClass {
protected:
    enum class myEnum{
        a,
        b,
        c
    };

    int fun(myClass::myEnum e);
};

int myClass::fun(myClass::myEnum e) {
    switch (e){
        case myEnum::a:{
            return 0;
        }
        case myEnum::b:{
            return 1;
        }
        case myEnum::c:{
            return 2;
        }
    }
}

Just by adding a third enum member (and handling it in switch) the range of valid enumerator values gets extended to 0..3 ([0..(2 ^ M - 1)] with M = 2) and clang happily accepts it without any complaints even though passing 3 into this function will miss the switch because compiler is not required to report UB either.

So the rule of thumb would be to write code in a manner that all paths end either with return throw or [[noreturn]] function. In this particular case I would probably wrote a single return statement with an assertion for unhandled enumerator values:

int myClass::fun(myClass::myEnum e) {
    int result{};
    switch (e){
        case myEnum::a:{
            result = 0;
            break;
        }
        case myEnum::b:{
            result = 1;
            break;
        }
        default:
        {
           assert(false);
           break;
        }
    }
    return result;
}
user7860670
  • 35,849
  • 4
  • 58
  • 84