From 8968b7b0a8bf265a6018682a854002421c2ea84c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 Sep 2017 02:22:43 -0400 Subject: [PATCH 1/3] test-line-buffer: simplify command parsing The handle_command() function matches an incoming command string with a sequence of starts_with() checks. But it also surrounds these with a switch on the first character of the command, which lets us jump to the right block of starts_with() without going linearly through the list. However, each case arm of the switch falls through to the one below it. This is pointless (we know that a command starting with 'b' does not need to check any of the commands in the 'c' block), and it makes gcc's -Wimplicit-fallthrough complain. We could solve this by adding a break at the end of each block. However, this optimization isn't helping anything. Even if it does make matching faster (which is debatable), this is code that is run only in the test suite, and each run receives at most two of these "commands". We should favor simplicity and readability over micro-optimizing. Instead, let's drop the switch statement completely and replace it with an if/else cascade. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/helper/test-line-buffer.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/t/helper/test-line-buffer.c b/t/helper/test-line-buffer.c index 81575fe2ab..078dd7e29d 100644 --- a/t/helper/test-line-buffer.c +++ b/t/helper/test-line-buffer.c @@ -17,27 +17,17 @@ static uint32_t strtouint32(const char *s) static void handle_command(const char *command, const char *arg, struct line_buffer *buf) { - switch (*command) { - case 'b': - if (starts_with(command, "binary ")) { - struct strbuf sb = STRBUF_INIT; - strbuf_addch(&sb, '>'); - buffer_read_binary(buf, &sb, strtouint32(arg)); - fwrite(sb.buf, 1, sb.len, stdout); - strbuf_release(&sb); - return; - } - case 'c': - if (starts_with(command, "copy ")) { - buffer_copy_bytes(buf, strtouint32(arg)); - return; - } - case 's': - if (starts_with(command, "skip ")) { - buffer_skip_bytes(buf, strtouint32(arg)); - return; - } - default: + if (starts_with(command, "binary ")) { + struct strbuf sb = STRBUF_INIT; + strbuf_addch(&sb, '>'); + buffer_read_binary(buf, &sb, strtouint32(arg)); + fwrite(sb.buf, 1, sb.len, stdout); + strbuf_release(&sb); + } else if (starts_with(command, "copy ")) { + buffer_copy_bytes(buf, strtouint32(arg)); + } else if (starts_with(command, "skip ")) { + buffer_skip_bytes(buf, strtouint32(arg)); + } else { die("unrecognized command: %s", command); } } From d0e9983980a25e0c398cc03342e5ad22ef85b8a8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 Sep 2017 02:23:24 -0400 Subject: [PATCH 2/3] curl_trace(): eliminate switch fallthrough Our trace handler is called by curl with a curl_infotype variable to interpret its data field. For most types we print the data and then break out of the switch. But for CURLINFO_TEXT, we print data and then fall through to the "default" case, which does the exact same thing (nothing!) that breaking out of the switch would. This is probably a leftover from an early iteration of the patch where the code after the switch _did_ do something interesting that was unique to the non-text case arms. But in its current form, this fallthrough is merely confusing (and causes gcc's -Wimplicit-fallthrough to complain). Let's make CURLINFO_TEXT like the other case arms, and push the default arm to the end where it's more obviously a catch-all. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index 9e40a465fd..713525f38e 100644 --- a/http.c +++ b/http.c @@ -638,9 +638,7 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, switch (type) { case CURLINFO_TEXT: trace_printf_key(&trace_curl, "== Info: %s", data); - default: /* we ignore unknown types by default */ - return 0; - + break; case CURLINFO_HEADER_OUT: text = "=> Send header"; curl_dump_header(text, (unsigned char *)data, size, DO_FILTER); @@ -665,6 +663,9 @@ static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, text = "<= Recv SSL data"; curl_dump_data(text, (unsigned char *)data, size); break; + + default: /* we ignore unknown types by default */ + return 0; } return 0; } From 1cf01a34eaccd6da613dba82291666db237916ab Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 21 Sep 2017 02:25:41 -0400 Subject: [PATCH 3/3] 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 Signed-off-by: Junio C Hamano --- apply.c | 3 ++- builtin/cat-file.c | 1 + builtin/checkout.c | 1 + builtin/remote-ext.c | 2 +- builtin/submodule--helper.c | 1 + config.c | 1 + convert.c | 3 ++- fsck.c | 1 + http-push.c | 1 + mailinfo.c | 1 + quote.c | 1 + read-cache.c | 1 + send-pack.c | 2 +- 13 files changed, 15 insertions(+), 4 deletions(-) diff --git a/apply.c b/apply.c index 71cbbd141c..c022af53a2 100644 --- a/apply.c +++ b/apply.c @@ -2920,6 +2920,7 @@ static int apply_one_fragment(struct apply_state *state, if (plen && (ws_rule & WS_BLANK_AT_EOF) && ws_blank_line(patch + 1, plen, ws_rule)) is_blank_context = 1; + /* fallthrough */ case '-': memcpy(old, patch + 1, plen); add_line_info(&preimage, old, plen, @@ -2927,7 +2928,7 @@ static int apply_one_fragment(struct apply_state *state, old += plen; if (first == '-') break; - /* Fall-through for ' ' */ + /* fallthrough */ case '+': /* --no-add does not add new lines */ if (first == '+' && state->no_add) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 4ccbfaac31..aee280ea2c 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -113,6 +113,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, if (textconv_object(path, obj_context.mode, &oid, 1, &buf, &size)) break; + /* else fallthrough */ case 'p': type = sha1_object_info(oid.hash, NULL); diff --git a/builtin/checkout.c b/builtin/checkout.c index 5c202b7af5..d091f06274 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -436,6 +436,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, * update paths in the work tree, and we cannot revert * them. */ + /* fallthrough */ case 0: return 0; default: diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c index bfb21ba7d2..6a9127a33c 100644 --- a/builtin/remote-ext.c +++ b/builtin/remote-ext.c @@ -57,7 +57,7 @@ static char *strip_escapes(const char *str, const char *service, special = str[rpos]; if (rpos == 1) break; - /* Fall-through to error. */ + /* fallthrough */ default: die("Bad remote-ext placeholder '%%%c'.", str[rpos]); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 818fe74f0a..f6a5e1af5d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1189,6 +1189,7 @@ static int push_check(int argc, const char **argv, const char *prefix) break; die("HEAD does not match the named branch in the superproject"); } + /* fallthrough */ default: die("src refspec '%s' must name a ref", rs->src); diff --git a/config.c b/config.c index cd5a69e630..08490dfe81 100644 --- a/config.c +++ b/config.c @@ -2353,6 +2353,7 @@ static int store_write_pair(int fd, const char *key, const char *value) case '"': case '\\': strbuf_addch(&sb, '\\'); + /* fallthrough */ default: strbuf_addch(&sb, value[i]); break; diff --git a/convert.c b/convert.c index a09935cb81..20d7ab67bd 100644 --- a/convert.c +++ b/convert.c @@ -1545,8 +1545,9 @@ static int ident_filter_fn(struct stream_filter *filter, switch (ident->state) { default: strbuf_add(&ident->left, head, ident->state); + /* fallthrough */ case IDENT_SKIPPING: - /* fallthru */ + /* fallthrough */ case IDENT_DRAINING: ident_drain(ident, &output, osize_p); } diff --git a/fsck.c b/fsck.c index 2d2d2e9432..2ad00fc4d0 100644 --- a/fsck.c +++ b/fsck.c @@ -588,6 +588,7 @@ static int fsck_tree(struct tree *item, struct fsck_options *options) case S_IFREG | 0664: if (!options->strict) break; + /* fallthrough */ default: has_bad_modes = 1; } diff --git a/http-push.c b/http-push.c index e4c9b065ce..d860c477c6 100644 --- a/http-push.c +++ b/http-push.c @@ -1523,6 +1523,7 @@ static int remote_exists(const char *path) break; case HTTP_ERROR: error("unable to access '%s': %s", url, curl_errorstr); + /* fallthrough */ default: ret = -1; } diff --git a/mailinfo.c b/mailinfo.c index f2387a3267..bcdbf98de5 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -822,6 +822,7 @@ static void handle_filter(struct mailinfo *mi, struct strbuf *line) if (!handle_commit_msg(mi, line)) break; mi->filter_stage++; + /* fallthrough */ case 1: handle_patch(mi, line); break; diff --git a/quote.c b/quote.c index 53b98a5b84..de2922ddd6 100644 --- a/quote.c +++ b/quote.c @@ -431,6 +431,7 @@ void tcl_quote_buf(struct strbuf *sb, const char *src) case '{': case '}': case '$': case '\\': case '"': strbuf_addch(sb, '\\'); + /* fallthrough */ default: strbuf_addch(sb, c); break; diff --git a/read-cache.c b/read-cache.c index b211c57af6..a051567610 100644 --- a/read-cache.c +++ b/read-cache.c @@ -220,6 +220,7 @@ static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st) case S_IFDIR: if (S_ISGITLINK(ce->ce_mode)) return ce_compare_gitlink(ce) ? DATA_CHANGED : 0; + /* else fallthrough */ default: return TYPE_CHANGED; } diff --git a/send-pack.c b/send-pack.c index b865f662e4..a8cc6b266e 100644 --- a/send-pack.c +++ b/send-pack.c @@ -497,7 +497,7 @@ int send_pack(struct send_pack_args *args, strbuf_release(&cap_buf); return atomic_push_failure(args, remote_refs, ref); } - /* Fallthrough for non atomic case. */ + /* else fallthrough */ default: continue; }