From 1ae7d991b493f2c896ae14f30eced5b1960163bc Mon Sep 17 00:00:00 2001 From: Chris Jefferson Date: Sun, 18 Jun 2017 23:20:35 +0100 Subject: [PATCH] Better define the result of MappingPermListList This fixes some bugs introduced in moving MappingPermListList into the kernel: * Crash when given negative integers. * Crash when given non-integer types. * Sometimes (incorrectly) returns a permutation when given two lists where no perm exists mapping one to the other. This function also alters the permutation generated by MappingPermListList to only move points in one of the input lists. This fixes a bug in RepresentativeAction, which assumed this was the case. --- lib/permutat.g | 9 +-- src/permutat.c | 93 ++++++++++++++++++++----- tst/testbugfix/2017-06-19-repaction.tst | 10 +++ tst/testinstall/perm.tst | 40 +++++++++++ 4 files changed, 130 insertions(+), 22 deletions(-) create mode 100644 tst/testbugfix/2017-06-19-repaction.tst diff --git a/lib/permutat.g b/lib/permutat.g index 4f4ad83eb0..e7d6708a84 100644 --- a/lib/permutat.g +++ b/lib/permutat.g @@ -510,7 +510,7 @@ end ); ## gap> PermList([1,2,4,5,3]); ## (3,4,5) ## gap> MappingPermListList([2,5,1,6],[7,12,8,2]); -## (1,8,5,12,11,10,9,6,2,7,4,3) +## (1,8,5,12,6,2,7) ## gap> RestrictedPerm((1,2)(3,4),[3..5]); ## (3,4) ## ]]> @@ -560,10 +560,11 @@ end); ## returns the permutation p from the ## previous sentence, i.e. src[i]^p = ## dst[i]. -## The permutation \pi fixes all points larger than the maximum of -## the entries in src and dst. +## The permutation \pi fixes any point which is not in src or +## dst. ## If there are several such permutations, it is not specified which of them -## returns. +## returns. If there is no such +## permutation, then returns fail. ## ## ## <#/GAPDoc> diff --git a/src/permutat.c b/src/permutat.c index 9b34634853..6200963934 100644 --- a/src/permutat.c +++ b/src/permutat.c @@ -4443,22 +4443,30 @@ Obj FuncMappingPermListList(Obj self, Obj src, Obj dst) Obj out; Obj tabdst, tabsrc; Int x; + Obj obj; Int mytabs[DEGREELIMITONSTACK+1]; Int mytabd[DEGREELIMITONSTACK+1]; + l = LEN_LIST(src); if (l != LEN_LIST(dst)) { - ErrorReturnVoid( "both arguments must be lists of equal length", - 0L, 0L, "type 'return;' or 'quit;' to exit break loop" ); - return 0L; + ErrorMayQuit( "Arguments must be lists of equal length", 0L, 0L); } d = 0; for (i = 1;i <= l;i++) { - x = INT_INTOBJ(ELM_LIST(src,i)); + obj = ELM_LIST(src, i); + if (!IS_POS_INTOBJ(obj)) { + ErrorMayQuit("First argument must be a list of positive integers", 0L, 0L); + } + x = INT_INTOBJ(obj); if (x > d) d = x; } for (i = 1;i <= l;i++) { - x = INT_INTOBJ(ELM_LIST(dst,i)); + obj = ELM_LIST(dst, i); + if (!IS_POS_INTOBJ(obj)) { + ErrorMayQuit("Second argument must be a list of positive integers", 0L, 0L); + } + x = INT_INTOBJ(obj); if (x > d) d = x; } if (d <= DEGREELIMITONSTACK) { @@ -4466,11 +4474,27 @@ Obj FuncMappingPermListList(Obj self, Obj src, Obj dst) memset(&mytabs,0,sizeof(mytabs)); memset(&mytabd,0,sizeof(mytabd)); for (i = 1;i <= l;i++) { - mytabs[INT_INTOBJ(ELM_LIST(src,i))] = i; + Int val = INT_INTOBJ(ELM_LIST(src, i)); + if (mytabs[val]) { + // Already read where this value maps, check it is the same + if (ELM_LIST(dst, mytabs[val]) != ELM_LIST(dst, i)) { + return Fail; + } + } + mytabs[val] = i; } for (i = 1;i <= l;i++) { - mytabd[INT_INTOBJ(ELM_LIST(dst,i))] = i; + Int val = INT_INTOBJ(ELM_LIST(dst, i)); + if (mytabd[val]) { + // Already read where this value is mapped from, check it is + // the same + if (ELM_LIST(src, mytabd[val]) != ELM_LIST(src, i)) { + return Fail; + } + } + mytabd[val] = i; } + out = NEW_PLIST(T_PLIST_CYC,d); SET_LEN_PLIST(out,d); /* No garbage collection from here ... */ @@ -4479,28 +4503,52 @@ Obj FuncMappingPermListList(Obj self, Obj src, Obj dst) if (mytabs[i]) { /* if i is in src */ SET_ELM_PLIST(out,i, ELM_LIST(dst,mytabs[i])); } else { - /* Skip things in dst: */ - while (mytabd[next]) next++; - SET_ELM_PLIST(out,i,INTOBJ_INT(next)); - next++; + if (mytabd[i]) { + // Skip things in dst: + while (mytabd[next] || + (mytabs[next] == 0 && mytabd[next] == 0)) + next++; + SET_ELM_PLIST(out, i, INTOBJ_INT(next)); + next++; + } + else { // map elements in neither list to themselves + SET_ELM_PLIST(out, i, INTOBJ_INT(i)); + } } } /* ... to here! No CHANGED_BAG needed since this is a new object! */ } else { /* Version with intermediate objects: */ - tabsrc = NEW_PLIST(T_PLIST,d); SET_LEN_PLIST(tabsrc,0); /* No garbage collection from here ... */ for (i = 1;i <= l;i++) { - SET_ELM_PLIST(tabsrc,INT_INTOBJ(ELM_LIST(src,i)),INTOBJ_INT(i)); + Int val = INT_INTOBJ(ELM_LIST(src, i)); + if (ELM_PLIST(tabsrc, val)) { + if (ELM_LIST(dst, INT_INTOBJ(ELM_PLIST(tabsrc, val))) != + ELM_LIST(dst, i)) { + return Fail; + } + } + else { + SET_ELM_PLIST(tabsrc, val, INTOBJ_INT(i)); + } } /* ... to here! No CHANGED_BAG needed since this is a new object! */ tabdst = NEW_PLIST(T_PLIST,d); SET_LEN_PLIST(tabdst,0); /* No garbage collection from here ... */ for (i = 1;i <= l;i++) { - SET_ELM_PLIST(tabdst,INT_INTOBJ(ELM_LIST(dst,i)),INTOBJ_INT(i)); + int val = INT_INTOBJ(ELM_LIST(dst, i)); + if (ELM_PLIST(tabdst, val)) { + if (ELM_LIST(src, INT_INTOBJ(ELM_PLIST(tabdst, val))) != + ELM_LIST(src, i)) { + return Fail; + } + } + else { + SET_ELM_PLIST(tabdst, val, INTOBJ_INT(i)); + } } /* ... to here! No CHANGED_BAG needed since this is a new object! */ out = NEW_PLIST(T_PLIST_CYC,d); @@ -4512,10 +4560,19 @@ Obj FuncMappingPermListList(Obj self, Obj src, Obj dst) SET_ELM_PLIST(out,i, ELM_LIST(dst,INT_INTOBJ(ELM_PLIST(tabsrc,i)))); } else { - /* Skip things in dst: */ - while (ELM_PLIST(tabdst,next)) next++; - SET_ELM_PLIST(out,i,INTOBJ_INT(next)); - next++; + if (ELM_PLIST(tabdst, i)) { + // Skip things in dst: + while (ELM_PLIST(tabdst, next) || + (ELM_PLIST(tabdst, next) == 0 && + ELM_PLIST(tabsrc, next) == 0)) { + next++; + } + SET_ELM_PLIST(out, i, INTOBJ_INT(next)); + next++; + } + else { + SET_ELM_PLIST(out, i, INTOBJ_INT(i)); + } } } /* ... to here! No CHANGED_BAG needed since this is a new object! */ diff --git a/tst/testbugfix/2017-06-19-repaction.tst b/tst/testbugfix/2017-06-19-repaction.tst new file mode 100644 index 0000000000..3ebc579372 --- /dev/null +++ b/tst/testbugfix/2017-06-19-repaction.tst @@ -0,0 +1,10 @@ +# These functions all worked incorrectly when given symmetric or alternating groups +# Which were not not defined on a domain of the form [1..n] +gap> RepresentativeAction(SymmetricGroup([5,7,11,15]),[7,11],[5,15],OnTuples); +(5,7)(11,15) +gap> RepresentativeAction(AlternatingGroup([5,7,11,15]),[7,11],[5,15],OnTuples); +(5,7)(11,15) +gap> RepresentativeAction(SymmetricGroup([5,7,11,15]),[7,11],[5,15],OnSets); +(5,7)(11,15) +gap> RepresentativeAction(AlternatingGroup([5,7,11,15]),[7,11],[5,15],OnSets); +(5,7)(11,15) diff --git a/tst/testinstall/perm.tst b/tst/testinstall/perm.tst index e2cec31313..33831e36d1 100644 --- a/tst/testinstall/perm.tst +++ b/tst/testinstall/perm.tst @@ -309,12 +309,52 @@ gap> List(permBig, SignPerm); # # MappingPermListList # +gap> MappingPermListList([],[]); +() gap> MappingPermListList([1,1], [2,2]); (1,2) gap> MappingPermListList([1,2], [2,1]); (1,2) gap> MappingPermListList([1,2], [3,4]); (1,3)(2,4) +gap> MappingPermListList([2,4,6], [1,2,3]); +(1,4,2)(3,6) +gap> MappingPermListList([1,1], [1,2]); +fail +gap> MappingPermListList([1,2], [1,1]); +fail +gap> MappingPermListList([1,1000], [1,1000]); +() +gap> MappingPermListList([1,1000], [1,1001]); +(1000,1001) +gap> MappingPermListList([1002,1000], [1,1001]); +(1,1000,1001,1002) +gap> MappingPermListList([1,1], [1000,1000]); +(1,1000) +gap> MappingPermListList([1,1], [1000,1001]); +fail +gap> MappingPermListList([1,2], [1000,1000]); +fail +gap> MappingPermListList((), []); +Error, Length: must be a list (not a permutation (small)) +gap> MappingPermListList([], ()); +Error, Length: must be a list (not a permutation (small)) +gap> MappingPermListList("cheese", "cake"); +Error, both arguments must be lists of equal length +gap> MappingPermListList("cheese", "cakeba"); +Error, Lists can only contain positive integers +gap> MappingPermListList([1,2], [3,[]]); +Error, Lists can only contain positive integers +gap> MappingPermListList([1,[]], [3,4]); +Error, Lists can only contain positive integers +gap> MappingPermListList([1,2], [3,0]); +Error, Lists can only contain positive integers +gap> MappingPermListList([1,0], [3,4]); +Error, Lists can only contain positive integers +gap> MappingPermListList([1,-1], [3,4]); +Error, Lists can only contain positive integers +gap> MappingPermListList([1,2], [3,4]); +(1,3)(2,4) gap> (1,128000) = (); false gap> (1,128000) * (129000, 129002);