hicpp-multiway-paths-covered complains about missing default when all cases are handled

[Quuxplusone]

Bugzilla Link	PR42929
Status	NEW
Importance	P normal
Reported by	Cristian Morales Vega (christian.morales.vega@gmail.com)
Reported on	2019-08-08 01:17:53 -0700
Last modified on	2020-10-15 01:01:17 -0700
Version	unspecified
Hardware	All All
CC	alexfh@google.com, arthur.j.odwyer@gmail.com, development@jonas-toth.eu, djasper@google.com, eugene.zelenko@gmail.com, klimek@google.com, lebedev.ri@gmail.com, madsravn@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

With clang-tidy 8.0.0

This code

$ cat la.c
enum test_enum {
    test_enum_1,
    test_enum_2,
    test_enum_3,
    test_enum_4
};

int test_switch(enum test_enum value)
{
    switch (value) {
        case test_enum_1:
            return 1;
        case test_enum_2:
            return 2;
        case test_enum_3:
            return 3;
        case test_enum_4:
            return 4;
    }
}

Generates this warning

$ clang-tidy -checks=hicpp-multiway-paths-covered la.c
...
la.c:10:5: warning: potential uncovered code path; add a default label [hicpp-
multiway-paths-covered]
    switch (value) {
    ^

If I add the default I will fail foul of

$ clang -Wcovered-switch-default -c -o la.o la.c

I have not looked in detauls, but AFAIU the code supposedly should not be
generating a warning there since CaseCount == MaxPathsPossible
(https://github.com/llvm-mirror/clang-tools-extra/blob/b1ace237b8a4c7246d6a3cd7b1d34118074d9af3/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp#L172)

[Quuxplusone]

TLDR switching on an enum is a big can of worms, user-experience-wise.

AFAIU the code supposedly should not be generating a warning there since CaseCount == MaxPathsPossible

I agree that this code probably shouldn't give a warning, but naively comparing the count of cases to the count of enumerator values isn't going to be useful at all. IMHO the thing to change would be where the code decides that the effective range of test_enum is the range of int: https://github.com/llvm-mirror/clang-tools-extra/blob/b1ace237b8a4c7246d6a3cd7b1d34118074d9af3/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp#L80-L90

Technically, if you see any value outside the range [0,4) for a test_enum, the program must have had undefined behavior already by the time you got to that line. (Only for old-style enums with no specified underlying type, though, I think?) So for example

// https://godbolt.org/z/SSqVIR enum TestEnumB { TEB_1 = 1, TEB_2 = 2, TEB_4 = 4 }; int TestSwitchB(TestEnumB value) { switch (value) { case TEB_1: return 1; case TEB_2: return 2; case TEB_1 | TEB_4: return 5; } }

Here we have 3 named enumerator values, and 3 switch cases, but it doesn't matter: we DO need to print the warning. Once we have all 8 cases covered, I think it would be defensible to stop printing the warning. However, if we had these 8 cases covered...

// https://godbolt.org/z/iFG8V6 enum TestEnumC { TEB_1 = 1, TEB_2 = 2, TEB_8 = 4 }; int TestSwitchC(TestEnumC value) { switch (value) { case 0: return 0; case TEB_1: return 1; case TEB_2: return 2; case TEB_1 | 2: return 3; case TEB_8: return 4; case TEB_1 | 8: return 5; case TEB_2 | 8: return 6; case TEB_1 | 2 | 8: return 7; } }

...then clearly the warning is needed again! (But both GCC and Clang give compiler warnings on this code, so maybe clang-tidy doesn't have to duplicate that effort?)

[Quuxplusone]

Very nice input, Arthur.

It do really seem like a big can of worms - and it's even worse that enum aren't 'type strong' compared to the underlying type. That you can switch on an enum and then make a case with 4 seems a little silly.

However, reading your response I am somehow leaning towards just removing this block of code if (CaseCount < MaxPathsPossible) { ... } to solve this bug. Then we will not receive a wrongful warning. To make the entire clang-tidy check correct would, as you show, require a lot of work, if possible.

What are your thoughts on this solution?

[Quuxplusone]

@Jonas Toth,

I am looking into this bug now. Can you confirm that this is not intended behavior? Reading your comments in the code and the assertions it seems like you are not interested in switches over enums.

[Quuxplusone]

Hey Mads,

you can of course look into this issue!
But during code-review we decided to not duplicate effort for stuff the
frontend already warns about.
I think there is a low chance that an existing warning-feature would be
implemented in clang-tidy, especially because clang-tidy can show the warnings
themself.
Additionally, improving the frontend warning would be worthwhile, too.

If there are already warnings about this kind of code, it could be documented
better in the clang-tidy docs, but otherwise I would be reluctant to re-
implement such a feature.

Best Regards, Jonas :)