Re: [PATCH v2 18/29] grep: catch a missing enum in switch statement

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2017, 05:50 UTC
Message-ID: <xmqqwp9ivfbu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170513231509.7834-19-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 11 quoted lines

> Add a die(...) to a default case for the switch statement selecting
> between grep pattern types under --recurse-submodules.
>
> Normally this would be caught by -Wswitch, but the grep_pattern_type
> type is converted to int by going through parse_options(). Changing
> the argument type passed to compile_submodule_options() won't work,
> the value will just get coerced. The -Wswitch-default warning will
> warn about it, but that produces a lot of noise across the codebase,
> this potential issue would be drowned in that noise.
>
> Thus catching this at runtime is the least worst option. This won't

least bad, I guess?

> ever trigger in practice, but if a new pattern type were to be added
> this catches an otherwise silent bug during development.

Good future-proofing. When this and Peff's "BUG" thing both graduates, we can turn this into a BUG, I think.

Thanks.

Show 22 quoted lines

>
> See commit 0281e487fd ("grep: optionally recurse into submodules",
> 2016-12-16) for the initial addition of this code.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/grep.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 3ffb5b4e81..a191e2976b 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -495,6 +495,8 @@ static void compile_submodule_options(const struct grep_opt *opt,
>  		break;
>  	case GREP_PATTERN_TYPE_UNSPECIFIED:
>  		break;
> +	default:
> +		die("BUG: Added a new grep pattern type without updating switch statement");
>  	}
>  
>  	for (pattern = opt->pattern_list; pattern != NULL;