On Tue, Sep 28 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
>> - 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 */
>> }
