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

From: Jeff King <peff@peff.net>
Date: Apr 20, 2017, 22:27 UTC
Message-ID: <20170420222747.ooo5z63tzi6d5ayl@sigill.intra.peff.net>
In-Reply-To: <20170420222016.GJ142567@google.com>

On Thu, Apr 20, 2017 at 03:20:16PM -0700, Brandon Williams wrote:

Show 20 quoted lines

> 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