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

Add "falls through" comments #536

Merged
merged 2 commits into from
Jun 7, 2017
Merged

Add "falls through" comments #536

merged 2 commits into from
Jun 7, 2017

Conversation

zsx
Copy link

@zsx zsx commented Jun 7, 2017

GCC 7 will have -Wimplicit-fallthrough turned on by -Wextra. There are a
number of ways to inform the compiler that falling through is intended.
This commit just uses the comment to do that, which is the most
conservative (least destructive) way: if the compiler doesn't recognize
it, nothing's gonna be broken.

zsx added 2 commits June 7, 2017 10:43
GCC 7 will have -Wimplicit-fallthrough turned on by -Wextra. There are a
number of ways to inform the compiler that falling through is intended.
This commit just uses the comment to do that, which is the most
conservative (least destructive) way: if the compiler doesn't recognize
it, nothing's gonna be broken.
Only new compilers (like GCC 7) support this option, so add
-Wno-unknown-warning in hope that the unrecoginized option will be
silently ignored, or -Werror will turn the warning into an error.
@zsx zsx requested a review from hostilefork June 7, 2017 14:47
@hostilefork
Copy link
Member

hostilefork commented Jun 7, 2017

Sure. Note I've been adding spaces whenever I can to:

switch (x) {
case A: {
     blah blah
     break; }
 case B: {
     blah blah
     break; }
  ...
  }

To produce:

switch (x) {
case A: {
     blah blah
     break; }

 case B: {
     blah blah
     break; }
  ...
  }

And using no spaces between cases for fallthrough only. That's been the standard I've been going for. It's tough to know exactly how to indent or how to write code, but I have come to like this pattern... because it's more or less readable and keeps rightward indentation from going out of control.

I don't have a problem with putting a fail or panic in the default case, if appropriate. Technically this creates slightly more overhead for the switch, but I'd have to see hard proof of it being a problem in the codegen of a particular instance before complaining too much.

(To me, part of "the project" is to keep the C code readable on 80 column terminals, it's all part of the "Amish programming" aesthetic... I want a surprisingly powerful engine to come from code that could have been understood circa the K&R C book...)

@zsx
Copy link
Author

zsx commented Jun 7, 2017

The style is good to me, however, GCC doesn't recognize it, and GCC 7 fails to compile with RIGOROUS=yes.

@hostilefork
Copy link
Member

Hm? Does it have a problem with the style I suggest or just if you leave out the // falls through comments? I am fine with the comments.

@zsx
Copy link
Author

zsx commented Jun 7, 2017

Sorry, I meant that it didn't recognize the style as a "falls through" indicator, so it didn't compile without the comments.

@hostilefork
Copy link
Member

Right. No problem, I like it. Just trying to elaborate on what my preferred style has become for switch statements--seems to be readable enough and keeps one from indenting too much e.g.

switch (x) {
    case A:
         {
             some code indented way too much...

@zsx zsx merged commit 4ae9c7a into master Jun 7, 2017
@zsx zsx deleted the fall-thru branch June 7, 2017 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants