Re: New-ish warning in refs.c with GCC (at least 11.2) under -O3

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 16, 2021, 20:29 UTC
Message-ID: <YZQUxkYI3TES3vDo@nand.local>
In-Reply-To: <YZLhrSoTzrC7wcQo@coredump.intra.peff.net>

On Mon, Nov 15, 2021 at 05:39:41PM -0500, Jeff King wrote:

> So something like the patch at the end of this email works (compiles
> with -O3 and passes the tests), but I think it is just making things
> more confusing.

I can absolutely understand and am sympathetic to the reasons that your patch would be making things more brittle. In some sense, it makes spots like these a little easier to read:

> -			&update->new_oid, &update->old_oid,
> +			update->flags & REF_HAVE_NEW ? &update->new_oid : NULL,
> +			update->flags & REF_HAVE_OLD ? &update->old_oid : NULL,

But I think forcing that burden on every caller is what makes the overall approach worse.

So it's too bad that we even have this problem in the first place, since GCC's warning is clearly a false positive. But I would be OK with the bandaid you propose here:

Show 9 quoted lines

> I think an assertion similar to what you have above is a better bet,
> but perhaps written more simply as:
>
>   if (flags & REF_HAVE_NEW) {
> 	/* silence gcc 11's over-eager compile-time analysis */
> 	if (!new_oid)
> 		BUG("REF_HAVE_NEW set without passing new_oid");
> 	oidcpy(&update->new_oid, new_oid);
>   }

Thanks, Taylor