Jeff King <peff@xxxxxxxx> writes: > The merge-recursive code uses stage number constants like this: > > add = &ci->ren1->dst_entry->stages[2 ^ 1]; > ... > add = &ci->ren2->dst_entry->stages[3 ^ 1]; > > The xor has the effect of flipping the "1" bit, so that "2 ^ 1" becomes > "3" and "3 ^ 1" becomes "2", which correspond to the "ours" and "theirs" > stages respectively. > > Unfortunately, clang-10 and up issue a warning for this code: > > merge-recursive.c:1759:40: error: result of '2 ^ 1' is 3; did you mean '1 << 1' (2)? [-Werror,-Wxor-used-as-pow] > add = &ci->ren1->dst_entry->stages[2 ^ 1]; > ~~^~~ > 1 << 1 > merge-recursive.c:1759:40: note: replace expression with '0x2 ^ 1' to silence this warning > > We could silence it by using 0x2, as the compiler mentions. Or by just > using the constants "2" and "3" directly. But after digging into it, I > do think this bit-flip is telling us something. If we just wrote: > > add = &ci->ren2->dst_entry->stages[2]; > > for the second one, you might think that "ren2" and "2" correspond. But > they don't. The logic is: ren2 is theirs, which is stage 3, but we > are interested in the opposite side's stage, so flip it to 2. So, the logical name for "^1" operator applied to 2 (ours) and 3 (theirs) is "the_other_side()"? the_other_side(theirs) == ours. I would have written (5 - side) instead of (side ^ 1) if I were writing this, though. > 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. Thanks.
