Re: [PATCH 1/2] avoid shifting signed integers 31 bits

From: Jeff King <peff@peff.net>
Date: Dec 30, 2015, 04:25 UTC
Message-ID: <20151230042519.GA5037@sigill.intra.peff.net>
In-Reply-To: <xmqq4mf0g5la.fsf@gitster.mtv.corp.google.com>

On Tue, Dec 29, 2015 at 04:09:21PM -0800, Junio C Hamano wrote:

Show 24 quoted lines

> > diff --git a/diff.h b/diff.h
> > index f7208ad..893f446 100644
> > --- a/diff.h
> > +++ b/diff.h
> > @@ -91,7 +91,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
> >  #define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
> >  #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
> >  #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
> > -#define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1 << 31)
> > +#define DIFF_OPT_DEFAULT_FOLLOW_RENAMES (1U << 31)
> >  
> >  #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
> >  #define DIFF_OPT_TOUCHED(opts, flag)    ((opts)->touched_flags & DIFF_OPT_##flag)
> 
> Thanks.
> 
> Seeing (1 << 30) and (1U <<31) together made me feel that we are way
> _too_ explicit being careful about 32-bit archs (iow, it would be
> more consistent to turn all of these "1 <<" into "1U <<"), but at
> the same time, (1 << 30) won't be broken unless we are on 31-bit
> arch in the sense that if we are on 30-bit or smaller arch the
> expression is already broken with or without "U", and if we are on
> 32-bit or more, then with or without "U" we are OK---which made me
> feel somewhat funny.

Yeah, I was tempted to convert them all, but there's no benefit except consistency. Interestingly, if we spelled these as:

that _is_ OK by the C standard (the type of the constant is "the first thing big enough to hold it" from a list of int, unsigned, etc[1]). I find that style more error-prone, though, so I don't think it's a good idea to move to it.

We pretty much assume an int of at least 32-bits anyway; the flags field itself is a simple "unsigned". We could make that a uint32_t, but in practice I hope that sub-32-bit platforms are all dead, at least for general purpose application code like git.

-Peff

[1] The list of possible types is actually _different_ for decimal and
    hex constants. Which seems slightly insane, but hey, it's C.
    Notably, the decimal equivalent of 0x80000000 is guaranteed to be
    signed (but would be a "long int" on a 32-bit platform).