Re: [PATCH v6 07/35] connect: convert get_remote_heads to use struct packet_reader
From: Jeff King <hidden>
Date: 2018-03-27 16:11:19
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Tue, Mar 27, 2018 at 05:27:14PM +0200, Duy Nguyen wrote:
quoted hunk ↗ jump to hunk
On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:quoted
In order to allow for better control flow when protocol_v2 is introduced +static enum protocol_version discover_version(struct packet_reader *reader) +{ + enum protocol_version version = protocol_unknown_version; + + /* + * Peek the first line of the server's response to + * determine the protocol version the server is speaking. + */ + switch (packet_reader_peek(reader)) { + case PACKET_READ_EOF: + die_initial_contact(0); + case PACKET_READ_FLUSH:gcc is dumb. When -Werror and -Wimplicit-fallthrough are enabled (on at least gcc 7.x), it fails to realize that this die_initial_contact() will not fall through (even though we do tell it about die() not returning, but I guess that involves more flow analysis to realize die_initial_contact is in the same boat). [...]@@ -124,6 +124,7 @@ enum protocol_version discover_version(struct packet_reader *reader) switch (packet_reader_peek(reader)) { case PACKET_READ_EOF: die_initial_contact(0); + break;
Would it make sense just to annotate that function to help the flow analysis? Like:
diff --git a/connect.c b/connect.c
index c3a014c5ba..49eca46462 100644
--- a/connect.c
+++ b/connect.c@@ -46,7 +46,7 @@ int check_ref_type(const struct ref *ref, int flags) return check_ref(ref->name, flags); } -static void die_initial_contact(int unexpected) +static NORETURN void die_initial_contact(int unexpected) { if (unexpected) die(_("The remote end hung up upon initial contact"));
That should let the callers know what's going on, and inside the function itself, the compiler should confirm that all code paths hit another NORETURN function. -Peff