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