Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add isFilter bit to kernel operation objects, and a IS_FILTER kernel function #1615

Closed
fingolfin opened this issue Aug 22, 2017 · 1 comment

Comments

@fingolfin
Copy link
Member

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)

@fingolfin fingolfin changed the title Add isFilter bit to kernel operation object, and a IS_FILTER kernel function Add isFilter bit to kernel operation objects, and a IS_FILTER kernel function Aug 22, 2017
@fingolfin
Copy link
Member Author

A global function IS_FILTER was added in #2732.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant