On Thu, Apr 20, 2017 at 03:20:16PM -0700, Brandon Williams wrote:
> On 04/20, Ævar Arnfjörð Bjarmason wrote:
> > 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.
Usually -Wswitch would catch this, but the variable in question is
declared as an int. The original pattern_type_arg variable is an int
because we pass its address via OPT_SET_INT().
We could make the argument to compile_submodule_options() an enum, but
then we get an implicit cast when we pass the int (which could fail at
runtime). Yech. I'm not sure if there's a good and easy solution.
-Peff