On Sun, Oct 23, 2016 at 07:57:30PM +0200, René Scharfe wrote: > > > Hard to trigger, but probably even harder to diagnose once someone > > > somehow manages to do it on some uncommon architecture. > > > > Indeed. If we are worried about overflow, we would also want to assume > > that it wraps at a multiple of 4, but that is probably a sane > > assumption. > > Hmm, I can't think of a way to violate this assumption except with unsigned > integers that are only a single bit wide. That would be a weird machine. > Are there other possibilities? No, I don't think so. I don't recall offhand whether the C standard allows integers that are not powers of 2. But if it does, and somebody develops such a platform, I have very little sympathy. My comment was mostly "this is the only other restriction I can think of, and it is crazy". > > You could also write the second line like: > > > > bufno %= ARRAY_SIZE(hexbuffer); > > > > which is less magical (right now the set of buffers must be a power of > > 2). I expect the compiler could turn that into a bitmask itself. > > Expelling magic is a good idea. And indeed, at least gcc, clang and icc on > x86-64 are smart enough to use an AND instead of dividing > (https://godbolt.org/g/rFPpzF). > > But gcc also adds a sign extension (cltq/cdqe) if we store the truncated > value, unlike the other two compilers. I don't see why -- the bit mask > operation enforces a value between 0 and 3 (inclusive) and the upper bits of > eax are zeroed automatically, so the cltq is effectively a noop. > > Using size_t gets us rid of the extra instruction and is the right type > anyway. It would suffice on its own, hmm.. Yeah, I had assumed you would also switch to some form of unsigned type either way. > > I'm fine with any of the options. I guess you'd want a similar patch for > > find_unique_abbrev on top of jk/no-looking-at-dotgit-outside-repo. > > Actually I'd want you to want to amend your series yourself. ;) Maybe I can > convince Coccinelle to handle that issue for us. I thought that series was in "next" already, but I see it isn't yet. I'd still wait until the sha1_to_hex() solution settles, and then copy it. > And there's also path.c::get_pathname(). That's enough cases to justify > adding a macro, I'd say: > [...] > +#define NEXT_RING_ITEM(array, index) \ > + (array)[(index) = ((index) + 1) % ARRAY_SIZE(array)] > + I dunno. It hides a lot of magic without saving a lot of lines in the caller, and the callers have to make sure "array" is an array and that "index" is unsigned. E.g., in this code: > @@ -24,8 +24,8 @@ static struct strbuf *get_pathname(void) > static struct strbuf pathname_array[4] = { > STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT > }; > - static int index; > - struct strbuf *sb = &pathname_array[3 & ++index]; > + static size_t index; > + struct strbuf *sb = &NEXT_RING_ITEM(pathname_array, index); > strbuf_reset(sb); > return sb; > } The truly ugly part is the repeated STRBUF_INIT. :) I think it would be preferable to just fix it inline in each place. -Peff