From 9a2e32e91d80e4eb71852523443d70c197aa969e Mon Sep 17 00:00:00 2001
From: Max Horn <max@quendi.de>
Date: Tue, 3 Aug 2021 00:34:34 +0200
Subject: [PATCH] Ranges: improve printing and conversion to strings

Also align the code for String/ViewString with the code of PrintObj for ranges.
---
 lib/list.gi                 | 52 ++++++++++++++++++++++---------------
 src/range.c                 | 13 ++++++----
 tst/testinstall/list.tst    |  2 +-
 tst/testinstall/range.tst   | 22 ++++++++--------
 tst/testinstall/reesmat.tst | 12 ++++-----
 5 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/lib/list.gi b/lib/list.gi
index 50f3cbc2120..f82ebd674e7 100644
--- a/lib/list.gi
+++ b/lib/list.gi
@@ -382,24 +382,36 @@ local   str,ls, i;
   return str;
 end );
 
-InstallMethod( String,
-    "for a range",
-    [ IsRange ],
+BindGlobal("STRING_FOR_RANGE",
     function( list )
     local   str;
-    str := Concatenation( "[ ", String( list[ 1 ] ) );
-    if Length( list ) > 1  then
-        if list[ 2 ] - list[ 1 ] <> 1  then
-            Append( str, ", " );
-            Append( str, String( list[ 2 ] ) );
-        fi;
+    str := "[ ";
+    Append( str, String( list[1] ) );
+    if Length( list ) > 1 then
+      if Length( list ) = 2 or list[2] - list[1] <> 1 then
+        Append( str, ", " );
+        Append( str, String( list[2] ) );
+      fi;
+      if Length( list ) > 2 then
         Append( str, " .. " );
         Append( str, String( list[ Length( list ) ] ) );
+      fi;
     fi;
     Append( str, " ]" );
+    Assert(0, IsStringRep(str));
     ConvertToStringRep( str );
     return str;
-    end );
+end );
+
+InstallMethod( ViewString,
+    "for a range",
+    [ IsRange ],
+    STRING_FOR_RANGE );
+
+InstallMethod( String,
+    "for a range",
+    [ IsRange ],
+    STRING_FOR_RANGE );
 
 
 #############################################################################
@@ -3829,7 +3841,7 @@ LIST_WITH_IDENTICAL_ENTRIES );
 ##  and in the 'ViewString' method for finite lists.
 ##
 InstallMethod( ViewObj,
-    "for finite lists",
+    "for a finite list",
     [ IsList and IsFinite ],
     {} -> RankFilter(IsList) + 1 - RankFilter(IsList and IsFinite),
 function( list )
@@ -3857,19 +3869,17 @@ function( list )
 end );
 
 InstallMethod( ViewObj,
-    "for ranges",
+    "for a range",
     [ IsList and IsFinite and IsRange ],
     function( list )
     Print( "[ " );
-    if   Length( list ) = 1 then
-      Print( list[1] );
-    elif Length( list ) = 2 then
-      Print( list[1], ", ", list[2] );
-    elif 2 < Length( list ) then
-      if list[2] - list[1] <> 1  then
-        Print( list[1], ", ", list[2], " .. ", list[ Length( list ) ] );
-      else
-        Print( list[1], " .. ", list[ Length( list ) ] );
+    Print( list[1] );
+    if Length( list ) > 1 then
+      if Length( list ) = 2 or list[2] - list[1] <> 1 then
+        Print( ", ", list[2] );
+      fi;
+      if Length( list ) > 2 then
+        Print( " .. ", list[ Length( list ) ] );
       fi;
     fi;
     Print( " ]" );
diff --git a/src/range.c b/src/range.c
index d71613e80de..ed6ea40b1ca 100644
--- a/src/range.c
+++ b/src/range.c
@@ -166,14 +166,17 @@ static Obj CopyRange(Obj list, Int mut)
 */
 static void PrintRange(Obj list)
 {
-    Pr( "%2>[ %2>%d",   
-       GET_LOW_RANGE(list), 0 );
+    Int len = GET_LEN_RANGE(list);
+    Pr( "%2>[ %2>%d",  GET_LOW_RANGE(list), 0 );
+    if ( len == 2 ) {
+        Pr( "%<,%< %2>%d%4< ]", GET_LOW_RANGE(list)+GET_INC_RANGE(list), 0);
+        return;
+    }
     if ( GET_INC_RANGE(list) != 1 ) {
-        Pr( "%<,%< %2>%d",
-           GET_LOW_RANGE(list)+GET_INC_RANGE(list), 0);
+        Pr( "%<,%< %2>%d", GET_LOW_RANGE(list)+GET_INC_RANGE(list), 0);
     }
     Pr( "%2< .. %2>%d%4< ]",
-       GET_LOW_RANGE(list)+(GET_LEN_RANGE(list)-1)*GET_INC_RANGE(list), 0 );
+       GET_LOW_RANGE(list)+(len-1)*GET_INC_RANGE(list), 0 );
 }
 
 
diff --git a/tst/testinstall/list.tst b/tst/testinstall/list.tst
index d59141b5515..cbeae59a9bf 100644
--- a/tst/testinstall/list.tst
+++ b/tst/testinstall/list.tst
@@ -383,7 +383,7 @@ gap> l := [10, 20];
 gap> IsRange(l);
 true
 gap> String(l);
-"[ 10, 20 .. 20 ]"
+"[ 10, 20 ]"
 gap> l := [2, 10, 18, 26, 34, 42];
 [ 2, 10, 18, 26, 34, 42 ]
 gap> IsRange(l);
diff --git a/tst/testinstall/range.tst b/tst/testinstall/range.tst
index 4079377447c..10e1221bfca 100644
--- a/tst/testinstall/range.tst
+++ b/tst/testinstall/range.tst
@@ -28,18 +28,18 @@ gap> TestPrintRangeRep:=function(r)
 #
 gap> TestPrintRangeRep([0..1]);
 ViewObj: [ 0, 1 ]
-PrintObj: [ 0 .. 1 ]
-Display: [ 0 .. 1 ]
+PrintObj: [ 0, 1 ]
+Display: [ 0, 1 ]
 DisplayString: <object>
 ViewString: [ 0, 1 ]
-PrintString: [ 0 .. 1 ]
-String: [ 0 .. 1 ]
+PrintString: [ 0, 1 ]
+String: [ 0, 1 ]
 gap> TestPrintRangeRep([0..2]);
 ViewObj: [ 0 .. 2 ]
 PrintObj: [ 0 .. 2 ]
 Display: [ 0 .. 2 ]
 DisplayString: <object>
-ViewString: [ 0, 1, 2 ]
+ViewString: [ 0 .. 2 ]
 PrintString: [ 0 .. 2 ]
 String: [ 0 .. 2 ]
 gap> TestPrintRangeRep([0,2..4]);
@@ -47,7 +47,7 @@ ViewObj: [ 0, 2 .. 4 ]
 PrintObj: [ 0, 2 .. 4 ]
 Display: [ 0, 2 .. 4 ]
 DisplayString: <object>
-ViewString: [ 0, 2, 4 ]
+ViewString: [ 0, 2 .. 4 ]
 PrintString: [ 0, 2 .. 4 ]
 String: [ 0, 2 .. 4 ]
 gap> TestPrintRangeRep([0,-1..-2]);
@@ -55,17 +55,17 @@ ViewObj: [ 0, -1 .. -2 ]
 PrintObj: [ 0, -1 .. -2 ]
 Display: [ 0, -1 .. -2 ]
 DisplayString: <object>
-ViewString: [ 0, -1, -2 ]
+ViewString: [ 0, -1 .. -2 ]
 PrintString: [ 0, -1 .. -2 ]
 String: [ 0, -1 .. -2 ]
 gap> TestPrintRangeRep([0,-1..-1]);
 ViewObj: [ 0, -1 ]
-PrintObj: [ 0, -1 .. -1 ]
-Display: [ 0, -1 .. -1 ]
+PrintObj: [ 0, -1 ]
+Display: [ 0, -1 ]
 DisplayString: <object>
 ViewString: [ 0, -1 ]
-PrintString: [ 0, -1 .. -1 ]
-String: [ 0, -1 .. -1 ]
+PrintString: [ 0, -1 ]
+String: [ 0, -1 ]
 
 #
 gap> 0 in [0..0];
diff --git a/tst/testinstall/reesmat.tst b/tst/testinstall/reesmat.tst
index d0ccf3750f6..fcb314adb1e 100644
--- a/tst/testinstall/reesmat.tst
+++ b/tst/testinstall/reesmat.tst
@@ -270,7 +270,7 @@ false
 
 # IsReesMatrixSemigroup: for a Rees matrix subsemigroup with generators, 1
 gap> R := ReesMatrixSemigroup(SymmetricGroup(2), [[(1,2)]]);
-<Rees matrix semigroup 1x1 over Sym( [ 1 .. 2 ] )>
+<Rees matrix semigroup 1x1 over Sym( [ 1, 2 ] )>
 gap> IsReesMatrixSemigroup(R);
 true
 gap> S := Semigroup(Elements(R));
@@ -302,7 +302,7 @@ false
 
 # IsReesZeroMatrixSemigroup: for a Rees matrix subsemigroup with generators, 1
 gap> R := ReesZeroMatrixSemigroup(SymmetricGroup(2), [[(1,2)]]);
-<Rees 0-matrix semigroup 1x1 over Sym( [ 1 .. 2 ] )>
+<Rees 0-matrix semigroup 1x1 over Sym( [ 1, 2 ] )>
 gap> IsReesZeroMatrixSemigroup(R);
 true
 gap> S := Semigroup(Elements(R));
@@ -320,13 +320,13 @@ false
 
 # IsReesZeroMatrixSemigroup: for a Rees matrix subsemigroup with generators, 1
 gap> R := ReesZeroMatrixSemigroup(SymmetricGroup(2), [[(1,2)]]);
-<Rees 0-matrix semigroup 1x1 over Sym( [ 1 .. 2 ] )>
+<Rees 0-matrix semigroup 1x1 over Sym( [ 1, 2 ] )>
 gap> S := Semigroup(RMSElement(R, 1, (1,2), 1), MultiplicativeZero(R));
 <subsemigroup of 1x1 Rees 0-matrix semigroup with 2 generators>
 gap> IsReesZeroMatrixSemigroup(S);
 false
 gap> R := ReesZeroMatrixSemigroup(SymmetricGroup(2), [[()]]);
-<Rees 0-matrix semigroup 1x1 over Sym( [ 1 .. 2 ] )>
+<Rees 0-matrix semigroup 1x1 over Sym( [ 1, 2 ] )>
 gap> S := Semigroup(RMSElement(R, 1, (), 1), MultiplicativeZero(R));
 <subsemigroup of 1x1 Rees 0-matrix semigroup with 2 generators>
 gap> IsReesZeroMatrixSemigroup(S);
@@ -402,7 +402,7 @@ infinity
 
 # Enumerator: for a Rees matrix semigroup
 gap> R := ReesMatrixSemigroup(SymmetricGroup(2), [[()]]);
-<Rees matrix semigroup 1x1 over Sym( [ 1 .. 2 ] )>
+<Rees matrix semigroup 1x1 over Sym( [ 1, 2 ] )>
 gap> enum := Enumerator(R);
 <enumerator of Rees matrix semigroup>
 gap> enum[1];
@@ -422,7 +422,7 @@ fail
 
 # Enumerator: for a Rees 0-matrix semigroup
 gap> R := ReesZeroMatrixSemigroup(SymmetricGroup(2), [[()]]);
-<Rees 0-matrix semigroup 1x1 over Sym( [ 1 .. 2 ] )>
+<Rees 0-matrix semigroup 1x1 over Sym( [ 1, 2 ] )>
 gap> enum := Enumerator(R);;
 gap> enum[Position(enum, enum[1])] = enum[1];
 true