On Tue, Mar 27, 2018 at 6:11 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Tue, Mar 27, 2018 at 05:27:14PM +0200, Duy Nguyen wrote:
>
>> On Thu, Mar 15, 2018 at 10:31:14AM -0700, Brandon Williams wrote:
>> > 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?
Yes that works wonderfully with my gcc-7.3.0
> 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
--
Duy
