Description
The following rambling is taken from PR #1593:
Hmm, I am not sure I understand [the code for IsFilter in lib/filter.g] (both old and new version).
[ For reference, this is the code in question: ]
# "old" version"
BIND_GLOBAL( "IsFilter",
x -> IS_IDENTICAL_OBJ(x, IS_OBJECT) or
( IS_OPERATION( x ) and ( FLAG1_FILTER( x ) <> 0 or x in FILTERS ) ) ) ;
# "new" version
BIND_GLOBAL( "IsFilter",
x -> IS_IDENTICAL_OBJ(x, IS_OBJECT)
or ( IS_OPERATION( x )
and ( (FLAG1_FILTER( x ) <> 0 and FLAGS_FILTER(x) <> false)
or x in FILTERS ) ) );Perhaps we can walk through it? (And perhaps the comment above it could be expanded to explain it, too?)
So first we handle IS_OBJECT/IsObject, which is special. OK. next we exclude everything that is not an operation, also fine.
But now in the old code, we look at FLAG1_FILTER( x ), and it is non-zero, we consider x a filter. Aha. But this is not quite right, as we have:
gap> FLAG1_FILTER(Setter(IsPGroup));
625
So I guess that's why you changed this to also requite FLAGS_FILTER(x) <> false ? How about instead or additional using NARG_FUNC(x)=1 ? I simply suggest that as it is immediately clear to me why that's a necessary condition for a filter, while FLAGS_FILTER(x) <> false isn't (simply because I have to look up what it does). But anyway, if using FLAGS_FILTER is "correct", that's also fine.
So... that leaves the question: Why do we need the final or x in FILTERS? Aha:
gap> Length(FILTERS);
2212
gap> Number(FILTERS, x -> IsOperation(x) and FLAG1_FILTER(x) = 0);
905
gap> foo:=Filtered(FILTERS, x -> IsOperation(x) and FLAG1_FILTER(x) = 0);;
gap> for i in [1..10] do Display(Random(foo)); od;
<Filter "HasUpperCentralSeriesOfGroup">
<Filter "HasDecomposedRationalClass">
<Filter "HasOrdersClassRepresentatives">
<Filter "HasCharacteristic">
<Filter "HasFpElmEqualityMethod">
<Filter "HasBlocksAttr">
<Filter "HasLargestElementGroup">
<Filter "HasAlternatingSubgroup">
<Filter "HasExp">
<Filter "HasTestSubnormallyMonomial">
gap> HasExp; Exp;
<Filter "HasExp">
<Attribute "Exp">
gap> HasAlternatingSubgroup; AlternatingSubgroup;
<Filter "HasAlternatingSubgroup">
<Attribute "AlternatingSubgroup">
Aha! So testers for attributes fall through:
gap> FLAG1_FILTER(HasExp);
0
gap> FLAG2_FILTER(HasExp);
308
gap> FLAGS_FILTER(HasExp);
<flag list>
Hmm...
gap> Number(FILTERS, x -> IsOperation(x) and FLAGS_FILTER(x) = false);
0
gap> FLAGS_FILTER(IS_OBJECT);
<flag list>
gap> FLAG1_FILTER(Exp);
0
gap> FLAG2_FILTER(Exp);
308
gap> FLAGS_FILTER(Exp);
<flag list>
At this point I got a headache, and started to wonder: Why don't we add a bit "isFilter" to the header of an operation, and add an IS_FILTER function to the kernel, which just returns that bit?
Note that we don't even need to grow the operation header for this, we could split ENABLED_ATTR into a bitfield. (I'd start by adding an OperationHeader struct, as described previously)