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

Hi Peff,

On Sun, 16 Oct 2016, Jeff King wrote:

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

quoted

quoted

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.

quoted

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@gmx.de/

Footnote *2*:
https://public-inbox.org/git/736bcb8e860c876e32e8f89f68b0b901abedc187.1472633606.git.johannes.schindelin@gmx.de/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.