On Thu, May 11, 2017 at 10:40 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Thu, May 11, 2017 at 1:08 PM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
>> On 05/11, Ævar Arnfjörð Bjarmason wrote:
>>> 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.
>>>
>>> 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 <avarab@xxxxxxxxx>
>>> ---
>>> builtin/grep.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/builtin/grep.c b/builtin/grep.c
>>> index 3ffb5b4e81..1c0adb30f3 100644
>>> --- a/builtin/grep.c
>>> +++ b/builtin/grep.c
>>> @@ -495,6 +495,12 @@ static void compile_submodule_options(const struct grep_opt *opt,
>>> break;
>>> case GREP_PATTERN_TYPE_UNSPECIFIED:
>>> break;
>>> + default:
>>> + /*
>>> + * -Wswitch doesn't catch this due to casting &
>>> + * -Wswitch-default is too noisy.
>>> + */
>>> + die("BUG: Added a new grep pattern type without updating switch statement");
>
> I am not sure if the comment is of enough value for the used screen
> real estate value.
> People interested in the existence of the default would just use
> blame/log to find out.
Thanks, I was on the fence about it, will remove it (and add the
mention of -Wswitch-default to the commit message).