Re: [PATCH 04/10] parse-options.c: use exhaustive "case" arms for "enum parse_opt_type"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 29, 2021, 08:48 UTC
Message-ID: <87tui3vk8y.fsf@evledraar.gmail.com>
In-Reply-To: <xmqq5yukgrlm.fsf@gitster.g>

On Tue, Sep 28 2021, Junio C Hamano wrote:

Show 19 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change code in get_value(), parse_options_check() etc. to do away with
>> the "default" case in favor of exhaustively checking the relevant
>> fields.
>
> I am not sure if this is a good idea in the bigger picture.
>
> If we know that unlisted cases should not happen, having "default:
> BUG()" without explicitly listing them is just as expressive as the
> result of this patch with much shorter code.
>
> When a new enum member is added without adding corresponding
> processing of the new value, either way it will be caught as a
> BUG().  Removing "default: BUG()" does allow you to catch such an
> error at compilation time, and keeping it may prevent you from doing
> so, but in practice, you'd be adding test coverage for the new case,
> which means that, even if your compiler is not cooperating, your
> test suite addition will hit "default: BUG();" in such a case.

Yes we'll catch them since these are loops over all options. But that assumes that:

1) You have a test that's stressing the relevant entry point

2) That you run all your tests, e.g. one of these entry points is the
   "git completion" one

I think listing the remaining enum arms is a small price to pay for having that all moved to compile-time.

Show 10 quoted lines

>> -	default:
>> +	/* special types */
>> +	case OPTION_END:
>> +	case OPTION_GROUP:
>> +	case OPTION_NUMBER:
>> +	case OPTION_ALIAS:
>>  		BUG("opt->type %d should not happen", opt->type);
>>  	}
>> +	return -1; /* gcc 10.2.1-6's -Werror=return-type */
>>  }