Re: [PATCH v3 03/18] grep: submodule-related case statements should die if new fields are added

From: Brandon Williams <bmwill@google.com>
Date: Apr 20, 2017, 22:20 UTC
Message-ID: <20170420222016.GJ142567@google.com>
In-Reply-To: <20170420212345.7408-4-avarab@gmail.com>

On 04/20, Ævar Arnfjörð Bjarmason wrote:

Show 14 quoted lines

> Change two case statements added in commit 0281e487fd ("grep:
> optionally recurse into submodules", 2016-12-16) so that they die if
> new GREP_PATTERN_* enum fields are added without updating them.
> 
> These case statements currently check for an exhaustive list of
> fields, but if a new field is added it's easy to introduce a bug here
> where the code will start subtly doing the wrong thing, e.g. if a new
> pattern type is added we'll fall through to
> GREP_PATTERN_TYPE_UNSPECIFIED, i.e. the "basic" POSIX regular
> expressions.
> 
> This should arguably be done for the switch(opt->binary)
> case-statement as well, but isn't trivial to add since that code isn't
> currently working with an exhaustive list.

I was under the impression that the code wouldn't compile if there is a missing enum field in the switch statement. Does it instead silently fall through? I would choose not compiling over a die statement that may not be caught during the development of a new series.

Show 31 quoted lines

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/grep.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 3ffb5b4e81..be3dbd6957 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;
> @@ -515,6 +517,8 @@ static void compile_submodule_options(const struct grep_opt *opt,
>  		case GREP_PATTERN_BODY:
>  		case GREP_PATTERN_HEAD:
>  			break;
> +		default:
> +			die("BUG: Added a new grep token type without updating case statement");
>  		}
>  	}
>  
> -- 
> 2.11.0
>

-- 
Brandon Williams