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

From: Jeff King <peff@peff.net>
Date: Oct 17, 2016, 09:36 UTC
Message-ID: <20161017093608.qfevdonb336mglgx@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1610171025470.197091@virtualbox>

[tl;dr: the version in your repo is fine, and there's a trivial fix
 below if we want to silence the warning in the meantime]

On Mon, Oct 17, 2016 at 10:37:52AM +0200, Johannes Schindelin wrote:

Show 7 quoted lines

> > 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.

I poked around a bit, and it seems we're both half-wrong. C99 says:

  6.7.2.2 Enumeration specifiers
  [...]
  The expression that defines the value of an enumeration constant shall
  be an integer constant expression that has a value representable as an
  int.
  [...]
  Each enumerated type shall be compatible with char, a signed integer
  type, or an unsigned integer type. The choice of type is
  implementation-defined, but shall be capable of representing the
  values of all the members of the enumeration.

My reading is that it can't be a single-bit bitfield as I claimed, but it also isn't necessarily interchangeable with an int. But you get at least a "char", and you can use all of those integer values even if they aren't explicitly part of the set. And I'd assume that goes for values even beyond the largest tag as long as you don't need more bits, so that:

  enum { A = 1, B = 2, C = 4 } x = A | B | C;

is OK (though I didn't see anything particularly about that in the standard).

Assigning "-1" works in the same way that normal "unsigned x = -1" works (and is defined by the standard), though of course it may unexpectedly conflict with an actual enum value if the compiler chooses a smaller type (e.g., it may literally be 255 in many cases).

Anyway. Enough language lawyering. It seems like clang is being overly strict in its interpretation of the standard (it should be giving us at least a char's worth of values). But it matters less what the standard says and more what real compilers do, and we have to deal with clang's behavior.

> 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.

Oh, I agree that the warning is annoying, and the code should not go away. We just need to figure out how to silence clang.

Show 9 quoted lines

> > 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).

Thanks, I was able to test that branch. It looks like clang is happy with it because you compare against the max value. Unlike the ARRAY_SIZE() check, this does mean if somebody modifies the enum without touching the array, we might go out of bounds. But things would be severely broken enough from the mismatch that I don't think it's worth worrying about too much (and I see you have a nice comment warning people about this).

If the rest of your interactive-rebase branch is coming soon, I think we can probably ignore it for now. Otherwise something like:

diff --git a/sequencer.c b/sequencer.c
index d662c6b..1fdc35e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -620,7 +620,8 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
 
 enum todo_command {
 	TODO_PICK = 0,
-	TODO_REVERT
+	TODO_REVERT,
+	TODO_MAX
 };
 

is probably the simplest portable fix.

As a more clever change, I wondered if switching the enum values from
(0,1) to (1,2) would silence the warning, and indeed it does. Which I
assume is because using bit-flags, we could now represent "1|2", or "3",
which is larger than the array (well, obviously "2" is, but we'd need to
subtract 1 when indexing the array). I don't think that's a good route,
though, because it loses the 0-indexing, the benefits of
zero-initialization, etc. I was mostly just poking at how clang
perceives the enum values.

> 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.

Heh. I gently hinted something similar to Eric in the past, but I think
he actually likes how it looks. He has invited others to mirror
public-inbox and make their own interface, though. I just lack the
"artistic touch" you mentioned.

-Peff