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

[Ævar Arnfjörð Bjarmason]

On Mon, May 15, 2017 at 7:50 AM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason   writes:
>
>> 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?

Will fix.

>> 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.

Yup, this and a bunch of other stuff presumably. The BUG feature is nice.

> Thanks.
>
>>
>> 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 
>> ---
>>  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;

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> 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.

>
> 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 
> ---
>  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;

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

[Ævar Arnfjörð Bjarmason]

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
ever trigger in practice, but if a new pattern type were to be added
this catches an otherwise silent bug during development.

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 
---
 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;
-- 
2.11.0