Make implicit fallthroughs a compiler error, remove unneeded breaks
ClosedPublic

Authored by aaronpuchert on Sep 22 2018, 4:49 PM.

Details

Summary

With -Wimplicit-fallthrough the compiler warns about unannotated
fallthroughs in switch statements. Since it is supported by both GCC and
Clang, and can easily be fixed by adding Q_FALLTHROUGH() we enable it as
error.

As a consequence, we don't need to add redundant break statements as
safety measure. So we also warn about that, but not as error, especially
since it's only supported on Clang. (As far as I know.)

Some of the fallthroughs are suspicious to me, maybe there is actually
an error.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
aaronpuchert created this revision.Sep 22 2018, 4:49 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptSep 22 2018, 4:49 PM
aaronpuchert requested review of this revision.Sep 22 2018, 4:49 PM

There are two suspicious fallthroughs, maybe someone with a better understanding than me can comment on that.

plugins/ghprovider/ghproviderwidget.cpp
168

Looks suspicious to me, is that intended?

plugins/qmljs/codecompletion/context.cpp
459 ↗(On Diff #42139)

Looks also suspicious.

@kossebau It seems you did something similar in D6301, maybe you can review this change. Should I also include qtcompat_p.h when Q_FALLTHROUGH is used?

@kossebau It seems you did something similar in D6301, maybe you can review this change. Should I also include qtcompat_p.h when Q_FALLTHROUGH is used?

Yes, needs to be included for such source file, because the min Qt dependency version is still 5.5, and Q_FALLTHROUGH was only introduced in 5.8 (see note at http://doc.qt.io/qt-5/qtglobal.html#Q_FALLTHROUGH)

Myself I never got really friends with the C++ switch statement, so have no personal style to defend here, fine to follow mainstream (as in, what compilers nudge us into :) ). So no objections to this patch.

No idea about the two suspicious places.

CMakeLists.txt
126

On GCC this flag is already enabled by -Wextra, which by something is already set for me.
What about other compilers?

Yes, needs to be included for such source file, because the min Qt dependency version is still 5.5, and Q_FALLTHROUGH was only introduced in 5.8 (see note at http://doc.qt.io/qt-5/qtglobal.html#Q_FALLTHROUGH)

I suspected that. I saw your change only after I pushed this, I will add the includes.

Myself I never got really friends with the C++ switch statement, so have no personal style to defend here, fine to follow mainstream (as in, what compilers nudge us into :) ). So no objections to this patch.

Switch statements are definitely weird, so I guess these warnings can be understood as enforcing a stricter (and more sensible) syntax.

CMakeLists.txt
126

I didn't see it with Clang. Clang doesn't support fallthrough comments like GCC does, and the standard [[fallthrough]] attribute is only available with C++17, maybe that's why it's not on by default.

aaronpuchert marked an inline comment as done.Sep 23 2018, 4:42 PM
aaronpuchert added subscribers: mssola, brauch.
aaronpuchert added inline comments.
plugins/ghprovider/ghproviderwidget.cpp
168

I'm pretty sure this is actually intended, maybe @mssola can confirm that.

plugins/qmljs/codecompletion/context.cpp
459 ↗(On Diff #42139)

So operator{Start,End} are used in functionCallTips to find the m_typeToMatch. The idea seems to be that if we write a + then we want something of the same type as a to add to it. With the fallthrough we apply this for commas as well, so we try to match the type before the comma. That doesn't seem right. I'm actually inclined to insert a break here. Looking at change R32:699603b11f3664e9e386d32add8496a0d3a8a7cc where this was introduced, it might have been an oversight.

@brauch Can you comment on that? The original author of the code doesn't seem to be on Phabricator, but you have apparently worked on QmlJs as well.

aaronpuchert marked an inline comment as done.

Remove comment in plugins/ghprovider/ghproviderwidget.cpp, as the fallthrough is probably intended, and add break in plugins/qmljs/codecompletion/context.cpp, as it is probably not intended. Now the code completion doesn't suggest variables of the same type as the last parameter in functions calls, which seems about right.

rjvbb added a subscriber: rjvbb.Sep 23 2018, 5:31 PM

As mentioned on another ticket, I don't like the idea of not putting breaks, no matter how clever the compiler and hard-coded choice of compiler options; I don't think you gain anything from it, and it doesn't improve code readability either for me (esp. when switch cases aren't indented w.r.t the switch context, as in the last hunk in plugins/qmljs/codecompletion/context.cpp.
Making it obligatory to add a token confirming that fallthrough is intended is a good idea, though using a largish term in all-caps like Q_FALLTHROUGH also doesn't improve readibilty. And what about the most common type of fallthrough, are we really going to have to write something like the following?

switch (type) {
    case TYPE_A:
        Q_FALLTHROUGH
    case TYPE_B:
        Q_FALLTHROUGH
    case TYPE_C:
        Q_FALLTHROUGH
    case TYPE_D:
        whatever();
        break;
    default:
        anyway();
        // remember not to put a break here
}

As mentioned on another ticket, I don't like the idea of not putting breaks, no matter how clever the compiler and hard-coded choice of compiler options.

The compiler doesn't need to be clever, it has to understand control flow anyway to compile the program. With Clang, you can actually see the generated control flow graph of your source file with the options --analyze -Xanalyzer -analyzer-checker=debug.DumpCFG.

I don't think you gain anything from it, and it doesn't improve code readability either for me

Can you explain in more detail how an unreachable break after a return statement makes the code more readable? Actually Framework: Syntax Highlighting distinguishes between control flow keywords and other keywords. Depending on your color scheme, you'll see if there is a terminator or not.

Making it obligatory to add a token confirming that fallthrough is intended is a good idea, though using a largish term in all-caps like Q_FALLTHROUGH also doesn't improve readibilty.

With C++17 you can use [[fallthrough]]; — until then we have to live with the all-caps. Apart from that, fallthroughs are not very common, and one could argue that they should be drawing a reader's attention.

And what about the most common type of fallthrough, are we really going to have to write something like the following?

When case labels follow directly after others, no annotation is necessary. Only when there is another statement in between. That doesn't happen very often, and if it does, the annotation certainly doesn't hurt.

switch (type) {
    case TYPE_A: // no fallthrough annotation needed
    case TYPE_B: // no fallthrough annotation needed
    case TYPE_C: // no fallthrough annotation needed
    case TYPE_D:
        whatever();
        break;  // annotation would be necessary if we didn't break
    default:
        anyway();
        // break optional
}
rjvbb added a comment.Sep 23 2018, 7:33 PM
Can you explain in more detail how an unreachable `break` after a `return` statement makes the code more readable?

It's a shortist word that's easily recognisable that serves as a closing tag. Remove it, and my first impression will be that there's a fallthrough (because I'm not that used to returns in a switch). And a default: without break feels ... incomplete, makes me wonder if I missed a brace.

until then we have to live with the all-caps.

I'll point out that we're no longer living with Q_FOREACH and that nothing forbids anyone to provide a less conspicuous pattern than Q_FALLTHROUGH.

Apart from that, fallthroughs are not very common, and one could argue that they should be drawing a reader's attention.

Maybe, but there's also a reason why we avoid all-caps in things that have to be easily readable.

When case labels follow directly after others, no annotation is necessary.

I don't disagree, but it *is* an exception that you can easily extend to other cases where it's evident that a fallthrough is intended:

switch (type) {
    case TYPE_A_OR_B:
       do_something_for_a_or_b();
       // it should be obvious that we're continuing straight to B
    case TYPE_B:
       do_something_for_b();
       break;

After all that mumbling in my beard (because that's what it is really) I wouldn't be doing that so much if we were encouraged (or obliged) to use braces systematically:

switch (type) {
    case A: {
        return theFavour;
    }
    case B: {
        catchB();
        Q_FALLTHROUGH;
    }
    default: {
        catchAll();
    }
}

I'll point out that we're no longer living with Q_FOREACH and that nothing forbids anyone to provide a less conspicuous pattern than Q_FALLTHROUGH.

Q_FALLTHROUGH has been picked up by many projects doing Qt-based software, also in codebase of KDE projects you already get > 100 hits (https://lxr.kde.org/search?_filestring=&_string=Q_FALLTHROUGH&_casesensitive=1).

Using Q_FALLTHROUGH, even if ugly from purist POV (which I also share), as a temporary solution until C++17 is the base, should be fine and helpful for people jumping between Qt-based projects. One's eye gets used to it, I can tell :) As we also got used to Q_SLOTS and Q_SIGNALS..

My +1 for Q_FALLTHROUGH. It will be not used that frequently, so even more important to follow common naming.

When case labels follow directly after others, no annotation is necessary.

I don't disagree, but it *is* an exception that you can easily extend to other cases where it's evident that a fallthrough is intended:

switch (type) {
    case TYPE_A_OR_B:
       do_something_for_a_or_b();
       // it should be obvious that we're continuing straight to B

This "obvious" is where code readers and code writers usually disagree, and where good code gets some clarifying comment. It's one of the pain points with the switch statement for me, so I completely understand why compilers and C++ standards nudge into certain directions.
A list of empty body cases (case A: case B: case C:) is a widespread pattern and non-ambiguous. Once a case gets some statement, there is less pattern, so it's better to be explicit.

When it comes to no break; after a return;, that also matches the idea to not have an else after an return in the branch before. I have read lots of code, and skipping the break; in that case does not ring bells for code reading confusion with me. Actually such lines with explicit unneeded break; only add noise to me, or worse, make think that line with the break; can still be reached somehow.
So my +1 for the code-cleanup as done in this patch.

After testing the GitHub provider and the QmlJS code completion I'm somewhat confident that these changes are correct, but I want to give the experts on that code a chance to weigh in before I merge this. Now onto the discussion.

It's a shortist word that's easily recognisable that serves as a closing tag. Remove it, and my first impression will be that there's a fallthrough (because I'm not that used to returns in a switch). And a default: without break feels ... incomplete, makes me wonder if I missed a brace.

As discussed earlier, switch statements do have a weird syntax and I'm completely with you if you don't like it. They are basically a glorified goto cascade. The case labels actually behave like goto labels in a lot of ways, which is the basis of oddities like Duff's device (which is a horrible idea, but it illustrates what switch statements can do). The normal way to exit a switch statement is obviously break, but another terminator does just fine as well, depending on what you want to do. It might take some time to get used to it, but I don't think it's crazy.

When case labels follow directly after others, no annotation is necessary.

I don't disagree, but it *is* an exception that you can easily extend to other cases where it's evident that a fallthrough is intended:

Like @kossebau, I'm not sure these obvious cases occur so often in practice. In the two cases here where I had to figure whether the fallthrough was intended, it took me quite some time, and in one of them I think it actually wasn't intended. I don't think of it as an exception to allow multiple case labels directly after each other, but more of a way to handle multiple cases in exactly the same way. Note that you can freely reorder the case labels of such a block. It's just grouping cases together.

That isn't happening in your example, the cases are actually handled differently. One case, after some preprocessing, "falls through" to the second.

After all that mumbling in my beard (because that's what it is really) I wouldn't be doing that so much if we were encouraged (or obliged) to use braces systematically:

If we got to redesign C from scratch, I would probably agree. But it's probably a bit too invasive for existing code bases, compared to adding a few annotations. That is the fine line compiler writers want to walk on, making switch statements behave more sanely while at the same time not completely changing everything about the language.

rjvbb added a comment.Sep 24 2018, 7:48 AM
When it comes to no `break;` after a `return;`, that also matches the idea to not have an `else` after an `return` in the branch before.

Not directly related, but in general I really prefer having as few exit points per function as possible. Sometimes you need some extra code for that, but it makes reading easier in the sense that you only have to worry about if's and else's to know if a certain location can be reached. Experience from debugging optimised code (about the only kind of debugging I do these days) strongly suggests that compilers strive to do generate a single exit point anyway.

Just to reflect on a remark made earlier: having "normal" colour vision (or colour printers/screens) shouldn't become a requirement for being able to read code.

rjvbb added a comment.Sep 24 2018, 8:27 AM
As discussed earlier, switch statements do have a weird syntax and I'm completely with you if you don't like it. They are basically a glorified `goto` cascade. The case labels actually behave like `goto` labels in a lot of ways, which is the basis of oddities like Duff's device

I can't say I don't like switch statements, I've done too much low-level C coding wishing I had had more justification to learn assembly, and for more powerful computed goto functionality (which a switch statement is, in a sense). My gripe is more with having to change my habits in ways I don't necessarily agree with entirely, and which might interfere with coding in other contexts.

Like @kossebau, I'm not sure these obvious cases occur so often in practice.

It may not in KDevelop's code, but that's not all that all of us work on. Coding style and guidelines for KDevelop itself could reflect good practice you'd like to advocate in general.

That isn't happening in your example, the cases are actually handled differently. One case, after some preprocessing, "falls through" to the second.

Yes and no. The example was meant to show a context where you have some sort of common denominator; think inheritance and a routine that does something like initialisation, where each case represents a less specialised version. Does that make it sound less exotic.
Re. Duff's device: I'm pretty certain I've seen a comparably obfuscated bit of code recently, possibly somewhere in the Qt code.

But it's probably a bit too invasive for existing code bases, compared to adding a few annotations.

Yes, that's why I used the concept of encouraging people ;)

Now just once more about using Q_FALLTHROUGH. I've wondered (but kept mum) about code reuse elsewhere, but there's also the question how much code there is in KDevelop that doesn't require any Qt APIs? If we agree that Q_FALLTHROUGH doesn't look very nice and is temporary only

Not that [[fallthrough]] looks so great (esp. when I'm not wearing appropriate sight correction :D) - I'd be curious to know how this will be handled in ObjC (where I think it corresponds to doing something "not-done" twice). BTW, will there be a corresponding C standard that introduces the same keyword, I'd surely hope so?! If not I do wonder if when C++ is going to introduce a switch/case version that's more on par with the version in interpreted languages that can take non-const cases...

My gripe is more with having to change my habits in ways I don't necessarily agree with entirely, and which might interfere with coding in other contexts.
It may not in KDevelop's code, but that's not all that all of us work on. Coding style and guidelines for KDevelop itself could reflect good practice you'd like to advocate in general.

I would actually advocate for this in general. I'm not sure which other code bases you work on, but I'm pretty sure both changes are consistent with the consensus in the C++ community. (The projects I work on outside of KDE do the same.) Otherwise compiler writers wouldn't implement these warnings. The fallthrough attribute has actually been standardized now.

Not that [[fallthrough]] looks so great (esp. when I'm not wearing appropriate sight correction :D) - I'd be curious to know how this will be handled in ObjC (where I think it corresponds to doing something "not-done" twice). BTW, will there be a corresponding C standard that introduces the same keyword, I'd surely hope so?! If not I do wonder if when C++ is going to introduce a switch/case version that's more on par with the version in interpreted languages that can take non-const cases...

You can use comments, or __attribute__((fallthrough)) in C. You probably want to hide the latter behind a macro, so that other compilers don't see it.

Regarding readability: With these changes, every case block ends with either a terminator or a fallthrough annotation. That is easy to verify both by humans and by compilers. And it means that the programmer has to make explicit, hence think about, what should happen when the block ends. I think this a better strategy to guard against accidental fallthroughs, because how do I know if it was intended if there is no annotation, no comment, no indication that it was? This way we are forcing the programmer to be explicit, so whatever happens will be the intended behavior. There is no way to accidentally miss a break anymore.

rjvbb added a comment.Sep 27 2018, 6:45 PM
You can use comments <https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/>, or `__attribute__((fallthrough))` in C.

That's strange decision if you ask me. I mean, if you're going to make the switch statement skeleton different for C and C++ (with a new keyword that's not available in both), why not renew it completely and rename it? I'd expect current (C++) compilers to be able to generate efficient code for a larger number of (const) variable types than the types one can use now. Strings for instance - like in that "looks like a switch" piece of code that got me involved here :)

Attributes are not keywords — they're a (C++-specific) generic syntax that was introduced with C++11. There are numerous compiler-specific attributes, usually prefixed with the compiler name, and some standardized attributes. See cppreference.com for a list of those.

Again, the idea is not starting from scratch, but introducing small changes that reduce the number of errors programmers make while requiring only minimal changes to the code. If you have to go over your code every few years and rewrite everything from scratch, not enough people will use the new standards. Introducing new syntax is usually the last resort, especially if an existing language mechanism can do the job as well.

rjvbb added a comment.Sep 28 2018, 7:51 AM
Attributes are not keywords — they're a (C++-specific) generic syntax that was introduced with C++11.
Again, the idea is not starting from scratch, but introducing small changes that reduce the number of errors programmers

With all due respect ... tomato tomato (I don't know phonetic spelling, sorry)...

My point here was that making a whatever-you-call it more or less obligatory in something that is (used to be) shared with a closely related language introduces unnecessary confusion ("wait, am I writing C or C++ here"). And extra overhead when reusing algorithms between those languages.
I don't see any good reason why the same syntax wouldn't be used in C. And I wasn't thinking of a complete overhaul of the syntax, only of the statement name, in this case ("multi" instead of "switch", for instance).

My point here was that making a whatever-you-call it more or less obligatory in something that is (used to be) shared with a closely related language introduces unnecessary confusion ("wait, am I writing C or C++ here"). And extra overhead when reusing algorithms between those languages.

C and C++ have substantially diverged over the years, and aren't really "closely related". They have shared roots, but not only has C++ added tons of features that C hasn't, C has also added features that C++ doesn't support. Consider designated initializers, variable-length arrays, and the restrict keyword. Compatibility between the two was never about the implementation, but about being able to share declarations (headers). Reusing algorithms was never on the table.

Even for headers the ability to reuse doesn't come for free, usually some preprocessor magic is required like

#ifdef __cplusplus
extern "C" {
#endif

I don't see any good reason why the same syntax wouldn't be used in C.

That's up to the C standard committee, not to the C++ committee, and certainly not up to us. I'd appreciate it if we limited the discussion to this change.

And I wasn't thinking of a complete overhaul of the syntax, only of the statement name, in this case ("multi" instead of "switch", for instance).

As I wrote, that requires changes to the language syntax including new reserved keywords that affect existing code. The attribute syntax doesn't require that, it's already there. But we're not the standards committee, so this is the wrong place to discuss such issues. If you don't like the direction C++ is taking, complain to them and not to me.

aaronpuchert edited reviewers, added: kossebau; removed: mssola, brauch.Oct 2 2018, 3:35 PM

@kossebau Are you Ok with this change? I'm not sure I'll get a review from the domain experts, but I'm fairly confident now that the changes are right.

kossebau added subscribers: kfunk, apol, mwolff.EditedOct 2 2018, 3:42 PM

Myself has no objections, rather positive on it.

Given this changes general flags/coding policy, would rather hope to see one from @mwolff @kfunk @brauch @apol give it a +1, too.

kfunk accepted this revision.Oct 2 2018, 7:45 PM
kfunk added inline comments.
plugins/qmljs/codecompletion/context.cpp
459 ↗(On Diff #42194)

Since this hunk is actually changing behavior, I'd commit it in a separate patch and also explain properly in the commit msg.

All other hunks are mainly cosmetic iiuc and can be committed in bulk.

This revision is now accepted and ready to land.Oct 2 2018, 7:45 PM
This revision was automatically updated to reflect the committed changes.
aaronpuchert marked an inline comment as done.
kossebau added inline comments.Oct 2 2018, 10:04 PM
plugins/qmljs/codecompletion/context.cpp
459 ↗(On Diff #42194)

@aaronpurchert This fix would be also candidate for 5.3 branch, right? If so, please cherry-pick -x it there, or tell me, I would do it then.

kossebau added inline comments.Oct 3 2018, 11:02 AM
plugins/qmljs/codecompletion/context.cpp
459 ↗(On Diff #42194)

Cherry-picked now to 5.3, so can be checked off :)