Skip to content

Commit

Permalink
Fix BlistList for two ranges
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
fingolfin committed Oct 6, 2019
1 parent 8492717 commit 3d9c302
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 13 deletions.
29 changes: 16 additions & 13 deletions src/blister.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);

}

Expand Down
20 changes: 20 additions & 0 deletions tst/testinstall/kernel/blister.tst
Original file line number Diff line number Diff line change
Expand Up @@ -149,15 +149,35 @@ gap> List(l, IsSSortedList);
gap> List(l, SizeBlist);
[ 1, 1, 2, 2 ]

#
# FuncBLIST_LIST
#
gap> BlistList(fail, fail);
Error, BlistList: <list> must be a small list (not the value 'fail')
gap> BlistList([1,2,3], fail);
Error, BlistList: <sub> 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: <list> must be a small list (not the value 'fail')
gap> ListBlist([1,2,3], fail);
Expand Down

0 comments on commit 3d9c302

Please sign in to comment.