Re: [PATCH 1/4] merge-recursive: silence -Wxor-used-as-pow warning

From: Elijah Newren <newren@gmail.com>
Date: Jan 25, 2020, 20:50 UTC
Message-ID: <CABPp-BFp3VrYjD8KTqnOff0-CDY7qO3Au7GeUDCuA46ofv1GSg@mail.gmail.com>
In-Reply-To: <20200125195515.GB5519@coredump.intra.peff.net>

On Sat, Jan 25, 2020 at 11:55 AM Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

>
> On Sat, Jan 25, 2020 at 09:27:36AM -0800, Junio C Hamano wrote:
>
> > > So let's keep the bit-flipping, but let's also put it behind a named
> > > function, which will make its purpose a bit clearer. This also has the
> > > side effect of suppressing the warning (and an optimizing compiler
> > > should be able to easily turn it into a constant as before).
> >
> > OK.  Now I see you named it flip_stage(), which is even better than
> > "the-other-side" above.  Makes sense.
> >
> > I still think ((2 + 3) - two_or_three_to_be_flipped) easier to
> > reason about than the bit flipping, as the implementation detail,
> > though.
>
> Yeah, the existing one relies on the coincidence that the two stages
> differ by a single bit (in another universe, they could well be stages
> "3" and "4").
>
> I don't overly care on the implementation either way, since it's now
> hidden in the helper. I mostly chose the bit-flip to match the existing
> code, but I'd be happy to change it. Other people who actually work on
> merge-recursive may have other opinions, though.

Interesting. In merge-ort (my in-development attempt at a replacement for merge-recursive), I'm currently storing the stages in an array with indices 0-2 rather than the 1-3 used by merge-recursive. This removes the empty/unused array entry at the beginning, and also works a bit better with the masks that traverse_trees() returns (as 1<<index corresponds to the bit in the mask from the traverse_trees() callback). In that scheme, bitflip won't work, but the subtraction idea still does. So, I'd tend to agree with Junio, but I think the helper you added here is probably the more important improvement.

However, all that said, I don't care all that much about what to do with merge-recursive in this case, because it currently looks like not much of the code is going to survive anyway. merge-ort isn't even alpha quality yet[1], but I seem to be moving towards totally different data structures and copying/sharing less and less code from merge-recursive with each change.

Elijah

[1] merge-ort still isn't functional yet other than in extremely narrow circumstances, I'm still experimenting with the data structures, and I've written several hundred lines of code and then thrown it all away at least once -- and may do so again. Whenever I find a useful patch I can separate and submit upstream, I have been doing so, but until the risk of another complete rewrite goes down, there's no point in me sending my half-baked ideas in for review. They need to be at least three-quarters baked first. :-)