From 8761c8f6f3ceb5abd5b0253a6c773caf5be2c39d Mon Sep 17 00:00:00 2001 From: Max Horn Date: Sun, 6 Oct 2019 14:36:37 +0200 Subject: [PATCH] Fix BlistList for two ranges Several results were wrong, and had been wrong since at least GAP 4.4. E.g. before this commit: gap> BlistList([0..3], [-3..-1]); [ true, true, true, false ] Now we get the correct result gap> BlistList([0,1,2,3], [-3..-1]); [ false, false, false, false ] One major issue was the use of unsigned variables to store signed data. Two variables were only of type `long` instead of `Int`, but on 64bit, this could lead to further issues. The logic handling the intersection was broken and overly complicated. The rewritten logic should be simpler. Finally, there is a tiny optimization enabled by switching from 1-based indexing to 0-based. --- src/blister.c | 29 ++++++++++++++++------------- tst/testinstall/kernel/blister.tst | 20 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/blister.c b/src/blister.c index a8fc87052c..d8c45bc0a7 100644 --- a/src/blister.c +++ b/src/blister.c @@ -1286,9 +1286,9 @@ static Obj FuncUNITE_BLIST_LIST(Obj self, Obj list, Obj blist, Obj sub) UInt bit; /* one bit of block */ Int lenList; /* logical length of the list */ const Obj * ptrSub; /* pointer to the sublist */ - UInt lenSub; /* logical length of sublist */ - UInt i, j, k = 0, l; /* loop variables */ - long s, t; /* elements of a range */ + Int lenSub; /* logical length of sublist */ + Int i, j, k, l; /* loop variables */ + Int s, t; /* elements of a range */ RequireSmallList("UniteBlistList", list); RequireBlist("UniteBlistList", blist); @@ -1313,19 +1313,22 @@ static Obj FuncUNITE_BLIST_LIST(Obj self, Obj list, Obj blist, Obj sub) /* get the bounds of the subset with respect to the boolean list */ s = INT_INTOBJ( GET_ELM_RANGE( list, 1 ) ); t = INT_INTOBJ( GET_ELM_RANGE( sub, 1 ) ); - if ( s <= t ) i = t - s + 1; - else i = 1; - if ( i + lenSub - 1 <= lenList ) j = i + lenSub - 1; - else j = lenList; + // compute bounds + i = t - s; + j = lenSub + i; + if (i < 0) + i = 0; + if (j > lenList) + j = lenList; /* set the corresponding entries to 'true' */ - for ( k = i; k <= j && (k-1)%BIPEB != 0; k++ ) - ptrBlist[(k-1)/BIPEB] |= (1UL << (k-1)%BIPEB); - for ( ; k+BIPEB <= j; k += BIPEB ) - ptrBlist[(k-1)/BIPEB] = ~(UInt)0; - for ( ; k <= j; k++ ) - ptrBlist[(k-1)/BIPEB] |= (1UL << (k-1)%BIPEB); + for ( k = i; k < j && k%BIPEB != 0; k++ ) + ptrBlist[k/BIPEB] |= (1UL << k%BIPEB); + for ( ; k+BIPEB < j; k += BIPEB ) + ptrBlist[k/BIPEB] = ~(UInt)0; + for ( ; k < j; k++ ) + ptrBlist[k/BIPEB] |= (1UL << k%BIPEB); } diff --git a/tst/testinstall/kernel/blister.tst b/tst/testinstall/kernel/blister.tst index c29c810c2d..62df417415 100644 --- a/tst/testinstall/kernel/blister.tst +++ b/tst/testinstall/kernel/blister.tst @@ -149,7 +149,9 @@ gap> List(l, IsSSortedList); gap> List(l, SizeBlist); [ 1, 1, 2, 2 ] +# # FuncBLIST_LIST +# gap> BlistList(fail, fail); Error, BlistList: must be a small list (not the value 'fail') gap> BlistList([1,2,3], fail); @@ -157,7 +159,25 @@ Error, BlistList: must be a small list (not the value 'fail') gap> BlistList([1,2,3], [1,3]); [ true, false, true ] +# ranges with increment 1 +gap> BlistList([0..3], [-3..-1]); +[ false, false, false, false ] +gap> BlistList([0..3], [-3..1]); +[ true, true, false, false ] +gap> BlistList([0..3], [-3..6]); +[ true, true, true, true ] +gap> BlistList([0..3], [0..1]); +[ true, true, false, false ] +gap> BlistList([0..3], [0..6]); +[ true, true, true, true ] +gap> BlistList([0..3], [2..6]); +[ false, false, true, true ] +gap> BlistList([0..3], [4..6]); +[ false, false, false, false ] + +# # FuncLIST_BLIST +# gap> ListBlist(fail, fail); Error, ListBlist: must be a small list (not the value 'fail') gap> ListBlist([1,2,3], fail);