Subject: Re: bug#55622: Bug in sort with keys and reverse, and version-sort and reverse

From: Paul Eggert <[email-masked]>
Date: Wed, 25 May 2022 11:28:21 -0700

Thanks, Pádraig, for fixing that. And thanks, Larry, for reporting that.

> The existing tests are sufficient to catch this.

Yes, evidently I forgot to run 'make check', which I usually do. I'll 
try to not forget next time....

I installed the attached further patches to (1) coalesce duplicate code 
and explain why it's needed and (2) tweak performance a tiny bit.
From 15627794459933d293547c2bf7d77ab196ae73a3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 25 May 2022 11:19:08 -0700
Subject: [PATCH 1/2] sort: refactor tricky diff reversal

* src/sort.c (diff_reversed): New function, to make the intent clearer.
(keycompare, compare): Use it.
---
 src/sort.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/sort.c b/src/sort.c
index dbe456038..0cd22f931 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -2601,6 +2601,15 @@ key_warnings (struct keyfield const *gkey, bool gkey_only)
     error (0, 0, _("option '-r' only applies to last-resort comparison"));
 }
 
+/* Return either the sense of DIFF or its reverse, depnding on REVERSED.
+   If REVERSED, do not simply negate DIFF as that can mishandle INT_MIN.  */
+
+static int
+diff_reversed (int diff, bool reversed)
+{
+  return reversed ? (diff < 0 ? 1 : -diff) : diff;
+}
+
 /* Compare two lines A and B trying every key in sequence until there
    are no more keys or a difference is found. */
 
@@ -2793,9 +2802,7 @@ keycompare (struct line const *a, struct line const *b)
         }
     }
 
-  if (key->reverse)
-    diff = diff < 0 ? 1 : -diff;
-  return diff;
+  return diff_reversed (diff, key->reverse);
 }
 
 /* Compare two lines A and B, returning negative, zero, or positive
@@ -2840,9 +2847,7 @@ compare (struct line const *a, struct line const *b)
         diff = (alen > blen) - (alen < blen);
     }
 
-  if (reverse)
-    diff = diff < 0 ? 1 : -diff;
-  return diff;
+  return diff_reversed (diff, reverse);
 }
 
 /* Write LINE to output stream FP; the output file's name is
-- 
2.34.1

From 85ddde23116e578768f90bad6899340da5394b75 Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email-masked]>
Date: Wed, 25 May 2022 11:23:39 -0700
Subject: [PATCH 2/2] sort: tune diff_reversed

* src/sort.c (diff_reversed): Tune.  On x86-64 with GCC, this
saves a conditional branch and shortens the generated machine code.
---
 src/sort.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sort.c b/src/sort.c
index 0cd22f931..0a6b557ac 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -2607,7 +2607,7 @@ key_warnings (struct keyfield const *gkey, bool gkey_only)
 static int
 diff_reversed (int diff, bool reversed)
 {
-  return reversed ? (diff < 0 ? 1 : -diff) : diff;
+  return reversed ? (diff < 0) - (diff > 0) : diff;
 }
 
 /* Compare two lines A and B trying every key in sequence until there
-- 
2.34.1