Hi Peff, On Sun, 16 Oct 2016, Jeff King wrote: > On Sun, Oct 16, 2016 at 10:09:12AM +0200, Johannes Schindelin wrote: > > > > 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. Really? I did see my share of code that completely violated this freedom, by assuming that it was really okay to cast a -1 to an enum and then test for that value later, when -1 was not a legal enum value. In any case, the fact that even one compiler used to build Git *may* violate that standard, and that we therefore need such safety guards as the one under discussion, still makes me think that this warning, while certainly well-intentioned, is poison for cross-platform projects. > > 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. "Later" is the sequencer-i patch series I already sent out for review [*1*], in particular the patch titled "sequencer (rebase -i): differentiate between comments and 'noop'" [*2*]. > 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. Junio chose to pick up only one patch series out of the rebase--helper thicket at a time, it seems. I did send out at least one revision per patch series prior to integrating them into Git for Windows v2.10.0, though. Plus, I kept updating the `interactive-rebase` branch in my repository on GitHub (https://github.com/dscho/git). Ciao, Dscho Footnote *1*: https://public-inbox.org/git/cover.1472633606.git.johannes.schindelin@xxxxxx/ Footnote *2*: https://public-inbox.org/git/736bcb8e860c876e32e8f89f68b0b901abedc187.1472633606.git.johannes.schindelin@xxxxxx/t/#u P.S.: I cannot wait for the day when somebody with an artistic touch provides .css for the public-inbox.org site so it stops threatening causing eye cancer to me.