Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

From: Jeff King <peff@peff.net>
Date: Oct 16, 2016, 19:42 UTC
Message-ID: <20161016194238.wbge2pas5xr46av7@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1610161006080.197091@virtualbox>

On Sun, Oct 16, 2016 at 10:09:12AM +0200, Johannes Schindelin wrote:

Show 9 quoted lines

> > Good catch. It technically needs to check the lower bound, too. In
> > theory, if somebody wanted to add an enum value that is negative, you'd
> > use a signed cast and check against both 0 and ARRAY_SIZE(). In
> > practice, that is nonsense for this case, and using an unsigned type
> > means that any negative values become large, and the check catches them.
> 
> I am pretty certain that I disagree with that warning: enums have been
> used as equivalents of ints for a long time, and will be, for a long time
> to come.

I'm not sure I agree. IIRC, Assigning values outside the range of an enum has always been fishy according to the standard, and a compiler really is allowed to allocate a single bit for storage for this enum.

> Given that this test is modified to `if (command < TODO_NOOP)` later, I
> hope that you agree that it is not worth the trouble to appease that
> compiler overreaction?

I don't mind if there are transient warnings on some compilers in the middle of a series, but I'm not sure when "later" is. The tip of "pu" has this warning right now when built with clang.

I'm happy to test the TODO_NOOP version against clang (and prepare a patch on top if it still complains), but that doesn't seem to have Junio's tree at all yet.

-Peff