@matheusbsbs / matheus-bagliano / commits / 1cf01a34ea

consistently use "fallthrough" comments in switches

Gcc 7 adds -Wimplicit-fallthrough, which can warn when a switch case falls through to the next case. The general idea is that the compiler can't tell if this was intentional or not, so you should annotate any intentional fall-throughs as such, leaving it to complain about any unannotated ones. There's a GNU __attribute__ which can be used for annotation, but of course we'd have to #ifdef it away on non-gcc compilers. Gcc will also recognize specially-formatted comments, which matches our current practice. Let's extend that practice to all of the unannotated sites (which I did look over and verify that they were behaving as intended). Ideally in each case we'd actually give some reasons in the comment about why we're falling through, or what we're falling through to. And gcc does support that with -Wimplicit-fallthrough=2, which relaxes the comment pattern matching to anything that contains "fallthrough" (or a variety of spelling variants). However, this isn't the default for -Wimplicit-fallthrough, nor for -Wextra. In the name of simplicity, it's probably better for us to support the default level, which requires "fallthrough" to be the only thing in the comment (modulo some window dressing like "else" and some punctuation; see the gcc manual for the complete set of patterns). This patch suppresses all warnings due to -Wimplicit-fallthrough. We might eventually want to add that to the DEVELOPER Makefile knob, but we should probably wait until gcc 7 is more widely adopted (since earlier versions will complain about the unknown warning type). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Sep 21, 2017 at 02:25 UTC 1cf01a34eaccd6da613dba82291666db237916ab

13 files changed +15 -4

apply.c

+2 -1

@@ -2920,6 +2920,7 @@ static int apply_one_fragment(struct apply_state *state,

2920 if (plen && (ws_rule & WS_BLANK_AT_EOF) &&

2921 ws_blank_line(patch + 1, plen, ws_rule))

2922 is_blank_context = 1;

2923 + /* fallthrough */

2924 case '-':

2925 memcpy(old, patch + 1, plen);

2926 add_line_info(&preimage, old, plen,

@@ -2927,7 +2928,7 @@ static int apply_one_fragment(struct apply_state *state,

2928 old += plen;

2929 if (first == '-')

2930 break;

2930 - /* Fall-through for ' ' */

2931 + /* fallthrough */

2932 case '+':

2933 /* --no-add does not add new lines */

2934 if (first == '+' && state->no_add)

builtin/cat-file.c

@@ -113,6 +113,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,

113

114 if (textconv_object(path, obj_context.mode, &oid, 1, &buf, &size))

115 break;

116 + /* else fallthrough */

117

118 case 'p':

119 type = sha1_object_info(oid.hash, NULL);

builtin/checkout.c

@@ -436,6 +436,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,

436 * update paths in the work tree, and we cannot revert

437 * them.

438 */

439 + /* fallthrough */

440 case 0:

441 return 0;

442 default:

builtin/remote-ext.c

+1 -1

@@ -57,7 +57,7 @@ static char *strip_escapes(const char *str, const char *service,

57 special = str[rpos];

58 if (rpos == 1)

59 break;

60 - /* Fall-through to error. */

60 + /* fallthrough */

61 default:

62 die("Bad remote-ext placeholder '%%%c'.",

63 str[rpos]);

builtin/submodule--helper.c

@@ -1189,6 +1189,7 @@ static int push_check(int argc, const char **argv, const char *prefix)

1189 break;

1190 die("HEAD does not match the named branch in the superproject");

1191 }

1192 + /* fallthrough */

1193 default:

1194 die("src refspec '%s' must name a ref",

1195 rs->src);

config.c

@@ -2353,6 +2353,7 @@ static int store_write_pair(int fd, const char *key, const char *value)

2353 case '"':

2354 case '\\':

2355 strbuf_addch(&sb, '\\');

2356 + /* fallthrough */

2357 default:

2358 strbuf_addch(&sb, value[i]);

2359 break;

convert.c

+2 -1

@@ -1545,8 +1545,9 @@ static int ident_filter_fn(struct stream_filter *filter,

1545 switch (ident->state) {

1546 default:

1547 strbuf_add(&ident->left, head, ident->state);

1548 + /* fallthrough */

1549 case IDENT_SKIPPING:

1549 - /* fallthru */

1550 + /* fallthrough */

1551 case IDENT_DRAINING:

1552 ident_drain(ident, &output, osize_p);

1553 }

fsck.c

@@ -588,6 +588,7 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)

588 case S_IFREG | 0664:

589 if (!options->strict)

590 break;

591 + /* fallthrough */

592 default:

593 has_bad_modes = 1;

594 }

http-push.c

@@ -1523,6 +1523,7 @@ static int remote_exists(const char *path)

1523 break;

1524 case HTTP_ERROR:

1525 error("unable to access '%s': %s", url, curl_errorstr);

1526 + /* fallthrough */

1527 default:

1528 ret = -1;

1529 }

mailinfo.c

@@ -822,6 +822,7 @@ static void handle_filter(struct mailinfo *mi, struct strbuf *line)

822 if (!handle_commit_msg(mi, line))

823 break;

824 mi->filter_stage++;

825 + /* fallthrough */

826 case 1:

827 handle_patch(mi, line);

828 break;

quote.c

@@ -431,6 +431,7 @@ void tcl_quote_buf(struct strbuf *sb, const char *src)

431 case '{': case '}':

432 case '$': case '\\': case '"':

433 strbuf_addch(sb, '\\');

434 + /* fallthrough */

435 default:

436 strbuf_addch(sb, c);

437 break;

read-cache.c

@@ -220,6 +220,7 @@ static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st)

220 case S_IFDIR:

221 if (S_ISGITLINK(ce->ce_mode))

222 return ce_compare_gitlink(ce) ? DATA_CHANGED : 0;

223 + /* else fallthrough */

224 default:

225 return TYPE_CHANGED;

226 }

send-pack.c

+1 -1

@@ -497,7 +497,7 @@ int send_pack(struct send_pack_args *args,

497 strbuf_release(&cap_buf);

498 return atomic_push_failure(args, remote_refs, ref);

499 }

500 - /* Fallthrough for non atomic case. */

500 + /* else fallthrough */

501 default:

502 continue;

503 }