Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider making it an error to implicitly fall through without __fallthrough; annotation in switch cases #43308

Closed
Gnbrkm41 opened this issue May 1, 2020 · 9 comments · Fixed by #43397
Assignees
Milestone

Comments

@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented May 1, 2020

I've just got bitten by implicit fallthroughs in #35713; That makes me wonder how hard it would be to make implicit fallthroughs be warnings / errors, or if it would be worth to do so?

The JIT coding convention states that fallthroughs should be annotated with __fallthrough; (https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/clr-jit-coding-conventions.md#124-switch-statements):

Fall-through between two case statements should be indicated with the __fallthrough; annotation. It should be surrounded by blank lines to make it maximally visible.

Given how we encourage people to flag fallthroughs explicitly and C# already makes it an error to omit break statements, would it be beneficial enough to make it an error/warning?

It appears that C++17 introduces [[fallthrough]]; attribute, and GNU/Clang recognises this when -Wimplicit-fallthrough argument is specified and makes these into warnings, but I'm not sure if VS has some flag to allow that, apart from this: https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-by-compiler-version?view=vs-2019#warnings-introduced-in-visual-studio-2017-rtm-compiler-version-1910250170

@stephentoub stephentoub transferred this issue from dotnet/runtime Oct 12, 2020
@sharwell
Copy link
Member

@stephentoub This is a C++ feature request. I'm not sure we can do anything about it here.

@stephentoub
Copy link
Member

Oops. Moved the wrong one. Thanks.

@stephentoub stephentoub transferred this issue from dotnet/roslyn Oct 12, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Meta untriaged New issue has not been triaged by the area owner labels Oct 12, 2020
@danmoseley
Copy link
Member

@Gnbrkm41 would you be interested in enabling the warning to see how many violations it flags?

@danmoseley
Copy link
Member

Setting coreclr label since this is largely a CoreCLR dev decision.

@Gnbrkm41
Copy link
Contributor Author

I'm not exactly sure how one would enable the analyzer. I heard that for MSVC it's part of the "C++ Core" Style checks (C++ Core Style Rules, ES.78) but presumably we don't want to enable the whole suite of analyzers?

@danmoseley
Copy link
Member

Hmm, I'll leave this to area owners to consider.

@jkotas
Copy link
Member

jkotas commented Oct 13, 2020

We can start with -Wimplicit-fallthrough and clang.

@tommcdon tommcdon added this to the 6.0.0 milestone Oct 13, 2020
@tommcdon tommcdon removed the untriaged New issue has not been triaged by the area owner label Oct 13, 2020
@janvorli
Copy link
Member

janvorli commented Oct 14, 2020

We can start with -Wimplicit-fallthrough and clang.

This would mean switching to C++17
Actually, I've tried that and clang seems to support it with C++11 as well.

@janvorli janvorli self-assigned this Oct 14, 2020
@janvorli
Copy link
Member

I've started working on it.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants