[1/4] merge-recursive: silence -Wxor-used-as-pow warning
diff mbox series

Message ID 20200125053723.GA744673@coredump.intra.peff.net
State New
Headers show
Series
  • more clang/sanitizer fixes
Related show

Commit Message

Jeff King Jan. 25, 2020, 5:37 a.m. UTC
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 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).

Signed-off-by: Jeff King <peff@peff.net>
---
It might be more readable still to #define STAGE_OURS and STAGE_THEIRS,
but the use of bare 2/3 is pervasive throughout the file. We'd probably
want to change it consistently, and perhaps call "ren1" "ren_ours" or
something like that. I'm not overly familiar with this code, so I tried
to keep it to what I found an obvious improvement. But I'm also happy to
go the "0x2 ^ 1" route if merge-recursive experts prefer that.

 merge-recursive.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Jan. 25, 2020, 5:27 p.m. UTC | #1
Jeff King <peff@peff.net> 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.
Jeff King Jan. 25, 2020, 7:55 p.m. UTC | #2
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.

-Peff
Elijah Newren Jan. 25, 2020, 8:50 p.m. UTC | #3
On Sat, Jan 25, 2020 at 11:55 AM Jeff King <peff@peff.net> wrote:
>
> 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.  :-)
Jeff King Jan. 25, 2020, 11:57 p.m. UTC | #4
On Sat, Jan 25, 2020 at 12:50:38PM -0800, Elijah Newren wrote:

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

OK, that's three people who think the subtraction is more obvious. I
agree it's not that big a deal now that it's hidden in the helper (and
the code may eventually go away anyway), but it's easy enough to fix it
while we're thinking about it.

Patch is below (as an incremental on top attributed to Junio, who
proposed it; but it would be fine to squash it in with a Helped-by,
too).

> [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.  :-)

Thank you, by the way, for all of the work you have done on
merge-recursive. I generally find it one of the scariest bits of the
code to look at, so I am happy to be able to punt it off to you. :)

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] merge-recursive: use subtraction to flip stage

The flip_stage() helper uses a bit-flipping xor to switch between "2"
and "3". While clever, this relies on a property of those two numbers
that is mostly coincidence. Let's write it as a subtraction; that's more
clear and would extend to other numbers if somebody copies the logic.

Signed-off-by: Jeff King <peff@peff.net>
---
 merge-recursive.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index e6aedd3cab..aee1769a7a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1713,12 +1713,11 @@ static char *find_path_for_conflict(struct merge_options *opt,
 }
 
 /*
- * Toggle the stage number between "ours" and "theirs" (2 and 3) by flipping
- * the 1-bit.
+ * Toggle the stage number between "ours" and "theirs" (2 and 3).
  */
 static inline int flip_stage(int stage)
 {
-	return stage ^ 1;
+	return (2 + 3) - stage;
 }
 
 static int handle_rename_rename_1to2(struct merge_options *opt,
Junio C Hamano Jan. 27, 2020, 7:17 p.m. UTC | #5
Jeff King <peff@peff.net> writes:

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

I don't, either, and as a step in this series, it makes perfect
sense to keep the implementation detail the same in the new helper.

Making it more readable would be a separate issue.

Thanks.

Patch
diff mbox series

diff --git a/merge-recursive.c b/merge-recursive.c
index 10dca5644b..e6aedd3cab 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1712,6 +1712,15 @@  static char *find_path_for_conflict(struct merge_options *opt,
 	return new_path;
 }
 
+/*
+ * Toggle the stage number between "ours" and "theirs" (2 and 3) by flipping
+ * the 1-bit.
+ */
+static inline int flip_stage(int stage)
+{
+	return stage ^ 1;
+}
+
 static int handle_rename_rename_1to2(struct merge_options *opt,
 				     struct rename_conflict_info *ci)
 {
@@ -1756,14 +1765,14 @@  static int handle_rename_rename_1to2(struct merge_options *opt,
 		 * such cases, we should keep the added file around,
 		 * resolving the conflict at that path in its favor.
 		 */
-		add = &ci->ren1->dst_entry->stages[2 ^ 1];
+		add = &ci->ren1->dst_entry->stages[flip_stage(2)];
 		if (is_valid(add)) {
 			if (update_file(opt, 0, add, a->path))
 				return -1;
 		}
 		else
 			remove_file_from_index(opt->repo->index, a->path);
-		add = &ci->ren2->dst_entry->stages[3 ^ 1];
+		add = &ci->ren2->dst_entry->stages[flip_stage(3)];
 		if (is_valid(add)) {
 			if (update_file(opt, 0, add, b->path))
 				return -1;
@@ -1776,7 +1785,7 @@  static int handle_rename_rename_1to2(struct merge_options *opt,
 		 * rename/add collision.  If not, we can write the file out
 		 * to the specified location.
 		 */
-		add = &ci->ren1->dst_entry->stages[2 ^ 1];
+		add = &ci->ren1->dst_entry->stages[flip_stage(2)];
 		if (is_valid(add)) {
 			add->path = mfi.blob.path = a->path;
 			if (handle_file_collision(opt, a->path,
@@ -1797,7 +1806,7 @@  static int handle_rename_rename_1to2(struct merge_options *opt,
 				return -1;
 		}
 
-		add = &ci->ren2->dst_entry->stages[3 ^ 1];
+		add = &ci->ren2->dst_entry->stages[flip_stage(3)];
 		if (is_valid(add)) {
 			add->path = mfi.blob.path = b->path;
 			if (handle_file_collision(opt, b->path,
@@ -1846,7 +1855,7 @@  static int handle_rename_rename_2to1(struct merge_options *opt,
 	path_side_1_desc = xstrfmt("version of %s from %s", path, a->path);
 	path_side_2_desc = xstrfmt("version of %s from %s", path, b->path);
 	ostage1 = ci->ren1->branch == opt->branch1 ? 3 : 2;
-	ostage2 = ostage1 ^ 1;
+	ostage2 = flip_stage(ostage1);
 	ci->ren1->src_entry->stages[ostage1].path = a->path;
 	ci->ren2->src_entry->stages[ostage2].path = b->path;
 	if (merge_mode_and_contents(opt, a, c1,