Re: [PATCH v5 5/5] convert: add "status=delayed" to filter process protocol

From: Jeff King <peff@peff.net>
Date: Jun 24, 2017, 20:36 UTC
Message-ID: <20170624203640.qomgbsakrgseungl@sigill.intra.peff.net>
In-Reply-To: <xmqq37ap9q2i.fsf@gitster.mtv.corp.google.com>

On Sat, Jun 24, 2017 at 11:51:49AM -0700, Junio C Hamano wrote:

Show 6 quoted lines

> >>  if ((CAP_DELAY & entry->supported_capabilities) &&
> >>      dco && dco->state == CE_CAN_DELAY))
> >
> > Agreed!
> 
> Why wasn't this caught earlier?  I thought this is something gcc warns about.

I thought so, too. If it warned about:

  if (A & B)

that would probably be too annoying. But:

  if (A & B && C)

is much more questionable. I wonder if it used to exist and gcc dropped it (or dropped it from -Wall). -Wlogical-op seems like the most likely candidate, but it does not catch it (and it has a false positive in handle_nonblock, or perhaps I just can't see the problem).

Show 10 quoted lines

> >> The operator precedence is such that it works without them, so this is
> >> just a style question (I'd also usually put the flags field before the
> >> flag itself, but that's really getting into aesthetics).
> >
> > You mean (entry & CAP_DELAY) instead of (CAP_DELAY & entry)?
> 
> Peff is continuing his explanation why (A & B && C) is technically
> correct and preferring ((A & B) && C) is purely stylistic.  "A & B"
> binds tighter than "something && C" which means that (A & B && C)
> cannot be misinterpreted as (A & (B && C)).

I actually meant both. The bitwise operator binds tighter so it's OK either way. But I would write "flags & MY_FLAG" and never "MY_FLAG & flags".

-Peff