From f8c4f0c8257554aa027ec35773b4f20281fd1c33 Mon Sep 17 00:00:00 2001 From: James Mitchell Date: Wed, 8 Feb 2017 22:21:42 +0000 Subject: [PATCH] kernel: fix bug in LtTrans24 in trans.c This commit fixes a bug in LtTrans24, a for loop was incorrectly nested. In order to increase the test coverage to include this part of the code (and a number of other parts of the code in trans.c that were previously unreachable) IdentityTrans is altered to be a T_TRANS4. Tests are added for this part of the code, and the other previously unreachable parts. --- src/trans.c | 124 ++++++++++++++++-------------- tst/testinstall/trans.tst | 156 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 57 deletions(-) diff --git a/src/trans.c b/src/trans.c index 8ad0fb0a26..33694afe7e 100644 --- a/src/trans.c +++ b/src/trans.c @@ -142,9 +142,9 @@ extern UInt INIT_TRANS4 (Obj f) { if (deg == 0) { // Special case for degree 0. - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. img = NEW_PLIST(T_PLIST_EMPTY + IMMUTABLE, 0); SET_LEN_PLIST(img, 0); @@ -1866,9 +1866,9 @@ Obj FuncPermLeftQuoTransformationNC (Obj self, Obj f, Obj g) { ptp[i] = ptg4[i]; } for (; i < def; i++) { - // This can't happen with transformations created within this file since - // a transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. ptp[ptf2[i]] = i; } } else if (TNUM_OBJ(f) == T_TRANS4 && TNUM_OBJ(g) == T_TRANS2) { @@ -1882,9 +1882,9 @@ Obj FuncPermLeftQuoTransformationNC (Obj self, Obj f, Obj g) { ptp[ptf4[i]] = ptg2[i]; } for (; i < deg; i++) { - // This can't happen with transformations created within this file since - // a transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. ptp[i] = ptg2[i]; } for (; i < def; i++) { @@ -2916,7 +2916,7 @@ Obj FuncCYCLES_TRANS_LIST (Obj self, Obj f, Obj list) { list_i = ELM_LIST(list, i); if (TNUM_OBJ(list_i) != T_INT || INT_INTOBJ(list_i) < 1) { ErrorQuit("CYCLES_TRANS_LIST: the second argument must be a " - "list of positive integer (not a %s)", + "list of positive integer (not a %s)", (Int) TNAM_OBJ(list_i), 0L); } j = INT_INTOBJ(list_i) - 1; @@ -3145,9 +3145,9 @@ Obj FuncTRANS_IMG_CONJ (Obj self, Obj f, Obj g) { // if def = min, then this isn't executed for (; i < def; i++) { - // This can't happen with transformations created within this file since - // a transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. ptsrc[ptf2[i]] = 1; ptdst[i] = 1; ptp[ptf2[i]] = i; @@ -3164,9 +3164,9 @@ Obj FuncTRANS_IMG_CONJ (Obj self, Obj f, Obj g) { // if deg = min, then this isn't executed for (; i < deg; i++) { - // This can't happen with transformations created within this file since - // a transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. //ptsrc[i] = 1; ptdst[ptg2[i]] = 1; ptp[i] = ptg2[i]; @@ -3643,9 +3643,9 @@ Int EqTrans24 (Obj f, Obj g) { } } } else { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < deg; i++) { if (*(ptf++) != *(ptg++)) { @@ -3673,9 +3673,9 @@ Int EqTrans42 (Obj f, Obj g) { deg = DEG_TRANS2(g); if (def <= deg) { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < def; i++) { if (* (ptf++) != * (ptg++)) { @@ -3789,9 +3789,9 @@ Int LtTrans24 (Obj f, Obj g) { } } } else { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < deg; i++) { if (ptf[i] != ptg[i]) { @@ -3801,13 +3801,13 @@ Int LtTrans24 (Obj f, Obj g) { return 0L; } } - for (; i < def; i++) { - if (ptf[i] != i) { - if (i > ptf[i]) { - return 1L; - } else { - return 0L; - } + } + for (; i < def; i++) { + if (ptf[i] != i) { + if (i > ptf[i]) { + return 1L; + } else { + return 0L; } } } @@ -3826,9 +3826,9 @@ Int LtTrans42 (Obj f, Obj g) { deg = DEG_TRANS2(g); if (def <= deg) { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < def; i++) { if (ptf[i] != ptg[i]) { @@ -3977,9 +3977,9 @@ Obj ProdTrans24 (Obj f, Obj g) { *ptfg++ = ptg[i]; } } else { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < def; i++) { *(ptfg++) = IMAGE(ptf[i], ptg, deg); @@ -3998,16 +3998,16 @@ Obj ProdTrans42 (Obj f, Obj g) { def = DEG_TRANS4(f); deg = DEG_TRANS2(g); - fg = NEW_TRANS4(def); + fg = NEW_TRANS4(MAX(def, deg)); ptfg = ADDR_TRANS4(fg); ptf = ADDR_TRANS4(f); ptg = ADDR_TRANS2(g); if (def <= deg) { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < def; i++) { *(ptfg++) = ptg[*(ptf++)]; @@ -4131,9 +4131,9 @@ Obj ProdTrans4Perm2 (Obj f, Obj p) { ptp = ADDR_PERM2(p); if (def <= dep) { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < def; i++) { *(ptfp++) = ptp[*(ptf++)]; } @@ -4230,9 +4230,9 @@ Obj ProdPerm2Trans4 (Obj p, Obj f) { *(ptpf++) = ptf[i]; } } else { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < dep; i++) { *(ptpf++) = IMAGE(ptp[i], ptf, def); } @@ -4376,9 +4376,9 @@ Obj PowTrans4Perm2 (Obj f, Obj p) { ptp = ADDR_PERM2(p); if (def == dep) { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < decnj; i++) { ptcnj[ptp[i]] = ptp[ptf[i]]; } @@ -4519,9 +4519,9 @@ Obj QuoTrans4Perm2 (Obj f, Obj p) { ptquo = ADDR_TRANS4(quo); if (def <= dep) { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (i = 0; i < def; i++) { *(ptquo++) = pttmp[*(ptf++)]; } @@ -4632,9 +4632,9 @@ Obj LQuoPerm2Trans4 (Obj opL, Obj opR) { ptM[p] = *(ptR++); } } else { - // This can't happen with transformations created within this file since a - // transformation is of type T_TRANS4 if and only if it has (internal) - // degree 65537 or greater. It is included to make the code more robust. + // The only transformation created within this file that is of type + // T_TRANS4 and that does not have (internal) degree 65537 or greater + // is ID_TRANS4. for (p = 0; p < degR; p++) { ptM[*(ptL++) ] = *(ptR++); } @@ -5340,6 +5340,16 @@ static Int InitLibrary ( StructInitInfo *module ) TmpTrans = 0; IdentityTrans = NEW_TRANS2(0); + // We make the next transformation to allow testing of some parts of the + // code which would not otherwise be accessible, since no other + // transformation created in this file is a T_TRANS4 unless its internal + // degree is > 65536. Such transformation can be created by packages with a + // kernel module, and so we introduce the next transformation for testing + // purposes. + Obj ID_TRANS4 = NEW_TRANS4(0); + AssGVar(GVarName("ID_TRANS4"), ID_TRANS4); + MakeReadOnlyGVar(GVarName("ID_TRANS4")); + /* return success */ return 0; } diff --git a/tst/testinstall/trans.tst b/tst/testinstall/trans.tst index be69450d9f..b0c369dc6f 100644 --- a/tst/testinstall/trans.tst +++ b/tst/testinstall/trans.tst @@ -292,6 +292,13 @@ true gap> IMAGE_SET_TRANS(Transformation([1], [65537])) > = [2 .. 65537]; true +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> IMAGE_SET_TRANS(f); +[ ] +gap> FLAT_KERNEL_TRANS(f); +[ ] # Test IMAGE_SET_TRANS_INT gap> IMAGE_SET_TRANS_INT(IdentityTransformation, -1); @@ -1824,6 +1831,13 @@ gap> TRANS_IMG_CONJ(f, LeftOne(f)); (1,6,4,12,11)(2,7,5,9)(3,8,10) gap> TRANS_IMG_CONJ(LeftOne(f), f); (1,11,12,4,6)(2,9,5,7)(3,10,8) +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> TRANS_IMG_CONJ(Transformation([2, 1, 3]), f); +(1,2) +gap> TRANS_IMG_CONJ(f, Transformation([2, 1, 3])); +(1,2) # One, IsOne, IdentityTransformation gap> f := Transformation([11, 9, 10, 6, 7, 7, 10, 7, 10, 9, 7, 4]);; @@ -1934,6 +1948,21 @@ gap> f = g; false gap> g = f; false +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> Transformation([1, 2, 3, 5, 4]) = f * (1, 2); +false +gap> f * (1, 2) = Transformation([1, 2, 3, 5, 4]); +false +gap> Transformation([2, 1, 3, 4, 5]) = f * (1, 2); +true +gap> f * (1, 2) = Transformation([2, 1, 3, 4, 5]); +true +gap> Transformation([2, 1, 3, 5, 4]) = f * (1, 2); +false +gap> f * (1, 2) = Transformation([2, 1, 3, 5, 4]); +false # \<, less than, LT gap> f := Transformation([8, 8, 2, 7, 9, 11, 7, 7, 6, 3, 1, 9]);; @@ -2072,6 +2101,45 @@ gap> f < g; false gap> g < f; true +gap> x := (2 ^ 16, 2 ^ 16 + 1); +(65536,65537) +gap> y := AsTransformation(x); + +gap> IsTrans4Rep(y); +true +gap> x := y ^ 2; +IdentityTransformation +gap> IsTrans4Rep(x); +true +gap> x < Transformation([2, 1, 3]); +true +gap> x < Transformation([1, 1, 3]); +false +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> Transformation([1, 2, 3, 5, 4]) < f * (1, 2); +true +gap> Transformation([3, 2, 3, 5, 4]) < f * (1, 2); +false +gap> f * (1, 2) < Transformation([1, 2, 3, 5, 4]); +false +gap> f * (1, 2) < Transformation([3, 2, 3, 5, 4]); +true +gap> f * (1, 2) < Transformation([1, 2, 3, 5, 4]); +false +gap> Transformation([2, 1, 3, 4, 5]) < f * (1, 2); +false +gap> Transformation([2, 1, 4, 4, 5]) < f * (1, 2); +false +gap> Transformation([2, 1, 1, 4, 5]) < f * (1, 2); +true +gap> f * (1, 2) < Transformation([2, 1, 3, 4, 5]); +false +gap> f * (1, 2) < Transformation([2, 1, 4, 4, 5]); +true +gap> f * (1, 2) < Transformation([2, 1, 1, 4, 5]); +false # \*, product, PROD: transformation and transformation gap> f := Transformation([8, 8, 2, 7, 9, 11, 7, 7, 6, 3, 1, 9, 13, 14]);; @@ -2106,6 +2174,31 @@ gap> g * f; Transformation( [ 2, 3, 2, 3 ] ) gap> f * g * f * g * f * g; Transformation( [ 2, 1, 1, 1 ] ) +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> Transformation([1, 2, 3, 5, 4]) * (f * (1, 2)); +Transformation( [ 2, 1, 3, 5, 4 ] ) +gap> Transformation([3, 2, 3, 5, 4]) * (f * (1, 2)); +Transformation( [ 3, 1, 3, 5, 4 ] ) +gap> (f * (1, 2)) * Transformation([1, 2, 3, 5, 4]); +Transformation( [ 2, 1, 3, 5, 4 ] ) +gap> (f * (1, 2)) * Transformation([3, 2, 3, 5, 4]); +Transformation( [ 2, 3, 3, 5, 4 ] ) +gap> (f * (1, 2)) * Transformation([1, 2, 3, 5, 4]); +Transformation( [ 2, 1, 3, 5, 4 ] ) +gap> Transformation([2, 1, 3, 4, 5]) * (f * (1, 2)); +IdentityTransformation +gap> Transformation([2, 1, 4, 4, 5]) * (f * (1, 2)); +Transformation( [ 1, 2, 4, 4 ] ) +gap> Transformation([2, 1, 1, 4, 5]) * (f * (1, 2)); +Transformation( [ 1, 2, 2 ] ) +gap> (f * (1, 2)) * Transformation([2, 1, 3, 4, 5]); +IdentityTransformation +gap> (f * (1, 2)) * Transformation([2, 1, 4, 4, 5]); +Transformation( [ 1, 2, 4, 4 ] ) +gap> (f * (1, 2)) * Transformation([2, 1, 1, 4, 5]); +Transformation( [ 1, 2, 1 ] ) # \*, PROD, product: for a transformation and a permutation gap> Transformation([1, 4, 2, 1]) * (1, 2); @@ -2159,6 +2252,30 @@ gap> f := Transformation([8, 1, 9, 7, 7, 6, 4, 2, 2, 4]);; gap> f * (1, 10, 2, 3, 6, 7)(11, 15)(12, 17, 19, 16, 20, 18); Transformation( [ 8, 10, 9, 1, 1, 7, 4, 3, 3, 4, 15, 17, 13, 14, 11, 20, 19, 12, 16, 18 ] ) +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> p := AS_PERM_TRANS(f);; +gap> IsPerm4Rep(p); +true +gap> Transformation([2, 1, 4, 4, 5]) * p; +Transformation( [ 2, 1, 4, 4 ] ) +gap> p * Transformation([2, 1, 4, 4, 5]); +Transformation( [ 2, 1, 4, 4 ] ) +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> (f * (1, 2)) * (1, 2, 3); +Transformation( [ 3, 2, 1 ] ) +gap> (1, 2, 3) * (f * (1, 2)); +Transformation( [ 1, 3, 2 ] ) +gap> p := AS_PERM_TRANS(f) * (1,2);; +gap> IsPerm4Rep(p); +true +gap> p * Transformation([2, 1, 4, 4, 5]); +Transformation( [ 1, 2, 4, 4 ] ) +gap> Transformation([2, 1, 4, 4, 5]) * p; +Transformation( [ 1, 2, 4, 4 ] ) # \*, PROD, product: for a permutation and a transformation gap> (1, 2) * Transformation([1, 4, 2, 1]); @@ -2239,6 +2356,16 @@ gap> f ^ p = p ^ -1 * f * p; true gap> Transformation([1, 2, 1]) ^ (1, 2, 3); Transformation( [ 2, 2 ] ) +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> p := AS_PERM_TRANS(f) * (1,2);; +gap> IsPerm4Rep(p); +true +gap> Transformation([2, 1]) ^ p; +Transformation( [ 2, 1 ] ) +gap> (f * (1, 2)) ^ (1, 2); +Transformation( [ 2, 1 ] ) # \/, quotient, QUO: for a transformation and a permutation gap> f := Transformation([8, 2, 6, 6, 7, 10, 8, 2, 1, 10]);; @@ -2306,6 +2433,16 @@ gap> OnTuples(MovedPoints(f), f); gap> Transformation([1], [65537]) / (1, 2) > = Transformation([1], [65537]) * (1, 2); true +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> p := AS_PERM_TRANS(f);; +gap> IsPerm4Rep(p); +true +gap> Transformation([2, 1]) / p; +Transformation( [ 2, 1 ] ) +gap> (f * (1, 2)) / (1, 2, 3); +Transformation( [ 1, 3, 2 ] ) # left quotient, LQUO: for a permutation and a transformation gap> f := Transformation([8, 2, 6, 6, 7, 10, 8, 2, 1, 10]);; @@ -2391,6 +2528,18 @@ gap> p := p * (1, 65538) ^ 2; (1,9,7,8,6,10,2,5,4,3) gap> LQUO(p, f) = p ^ -1 * f; true +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> p := AS_PERM_TRANS(f);; +gap> IsPerm4Rep(p); +true +gap> LQUO(p, Transformation([2, 1])); +Transformation( [ 2, 1 ] ) +gap> LQUO(p * (1, 2), Transformation([2, 1])); +IdentityTransformation +gap> LQUO((1,2,3), f * (1, 2)); +Transformation( [ 3, 2, 1 ] ) # ^, POW: for a positive integer and a transformation gap> 2 ^ Transformation([1, 1]); @@ -2835,6 +2984,13 @@ Error, PermLeftQuoTransformation: usage, the arguments must have equal image set and kernel, gap> PermLeftQuoTransformation(f, f * (2,11,5,6,9)); (2,11,5,6,9) +gap> f := ID_TRANS4;; +gap> IsTrans4Rep(f); +true +gap> PermLeftQuoTransformation(Transformation([2, 1, 3]), f); +(1,2) +gap> PermLeftQuoTransformation(f, Transformation([2, 1, 3])); +(1,2) # String gap> String(Transformation([2, 6, 7, 2, 6, 9, 9, 1, 11, 1, 12, 5]));