Skip to content

Refactor DeclareAttribute and NewAttribute #2091

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.

Sign up for GitHub

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

Merged
fingolfin merged 5 commits into from
Jan 23, 2018
Merged

Refactor DeclareAttribute and NewAttribute #2091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
36d02d2
Refactor DeclareAttribute and fix a parameter parsing bug.
markuspf Jan 10, 2018
aeda08f
Sync hpcgap/lib/oper.g
markuspf Jan 12, 2018
fb4f8c0
Update documentation of `DeclareAttribute`
markuspf Jan 11, 2018
141349e
Sync documentation of DeclareAttribute to HPC-GAP
markuspf Jan 11, 2018
60ad35d
Add tests for the argument parsing code of NewAttribute/DeclareAttribute
Jan 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
240 changes: 129 additions & 111 deletions hpcgap/lib/oper.g
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,7 +1095,7 @@ end );

#############################################################################
##
#F NewAttribute( <name>, <filter>[, "mutable"][, <rank>] ) . . new attribute
#F NewAttribute( <name>, <filter>[, <mutable>][, <rank>] ) . . new attribute
##
## <#GAPDoc Label="NewAttribute">
## <ManSection>
Expand All @@ -1117,16 +1117,23 @@ end );
## applicable to a character table,
## which is neither a list nor a collection.
## <P/>
## If the optional third argument is given then there are two possibilities.
## Either it is an integer <A>rank</A>,
## then the attribute tester has this incremental rank
## (see&nbsp;<Ref Sect="Filters"/>).
## Or it is the string <C>"mutable"</C>,
## then the values of the attribute shall be mutable;
## more precisely, when a value of such a mutable attribute is set
## then this value itself is stored, not an immutable copy of it.
## (So it is the user's responsibility to set an object that is in fact
## mutable.)
## For the optional third and fourth arguments, there are the following
## possibilities.
## <List>
Copy link
Member

@fingolfin fingolfin Jan 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a list? And why talk about 3rd and 4th together? Perhaps this was because it is how one thinks about it when implementing it (i.e. "how do I distinguish which of the two args the user meant?), but somebody reading the docs cares little about it (and can easily guess it anyway, as the types of the two args are distinct).

How about something more like this:

If the argument is given, it must be a positive integer, which is used as incremental rank (see BLA). If not given, then 1 is used.

If the argument is given, and either equal to the string "mutable", or to the boolean true, then ...
If is false or not given, then...

## <Item> The integer argument <A>rank</A> causes the attribute tester to have
## this incremental rank (see&nbsp;<Ref Sect="Filters"/>),
## </Item>
## <Item> If the argument <A>mutable</A> is the string <C>"mutable"</C> or
## the boolean <C>true</C>, then the values of the attribute are mutable.
## </Item>
## <Item> If the argument <A>mutable</A> is the boolean <C>false</C>, then
Copy link
Member

@fingolfin fingolfin Jan 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'd rather say: "Otherwise, i.e. if mutable is not given or set to false,"

## the values of the attribute are immutable.
## </Item>
## </List>
## <P/>
## When a value of such mutable attribute is set
## then this value itself is stored, not an immutable copy of it,
## and it is the user's responsibility to set an object that is mutable.
## This is useful for an attribute whose value is some partial information
## that may be completed later.
## For example, there is an attribute <C>ComputedSylowSubgroups</C>
Expand All @@ -1139,7 +1146,7 @@ end );
## <!-- attributes; is this really intended?-->
## <!-- if yes then it should be documented!-->
## <P/>
## If no third argument is given then the rank of the tester is 1.
## If no argument for <A>rank</A> is given, then the rank of the tester is 1.
## <P/>
## Each method for the new attribute that does <E>not</E> require
## its argument to lie in <A>filter</A> must be installed using
Expand Down Expand Up @@ -1179,33 +1186,43 @@ BIND_GLOBAL( "OPER_SetupAttribute", function(getter, flags, mutflag, filter, ran
return;
end);

# construct getter, setter and tester
BIND_GLOBAL( "NewAttribute", function ( name, filter, args... )
local flags, mutflag, getter, rank;

BIND_GLOBAL( "NewAttribute", function ( arg )
local name, filter, flags, mutflag, getter, rank;
if not IS_STRING( name ) then
Error( "<name> must be a string");
fi;

# construct getter, setter and tester
name := arg[1];
filter := arg[2];
if not IsFilter( filter ) then
Error( "<filter> must be a filter" );
fi;

if not IS_OPERATION( filter ) then
Error( "<filter> must be an operation" );
rank := 1;
mutflag := false;
if LEN_LIST(args) = 0 then
# this is fine, but does nothing
elif LEN_LIST(args) = 1 and args[1] in [ "mutable", true, false ] then
mutflag := args[1] in [ "mutable", true];
elif LEN_LIST(args) = 1 and IS_INT(args[1]) then
rank := args[1];
elif LEN_LIST(args) = 2
and args[1] in [ "mutable", true, false ]
and IS_INT(args[2]) then
mutflag := args[1] in [ "mutable", true ];
rank := args[2];
else
Error("Usage: NewAttribute( <name>, <filter>[, <mutable>][, <rank>] )");
fi;
flags:= FLAGS_FILTER( filter );

# the mutability flags is the third one (which can also be the rank)
mutflag := LEN_LIST(arg) = 3 and arg[3] = "mutable";
flags:= FLAGS_FILTER( filter );

# construct a new attribute
if mutflag then
getter := NEW_MUTABLE_ATTRIBUTE( name );
else
getter := NEW_ATTRIBUTE( name );
fi;
if LEN_LIST(arg) = 3 and IS_INT(arg[3]) then
rank := arg[3];
else
rank := 1;
fi;
STORE_OPER_FLAGS(getter, [ flags ]);

atomic FILTER_REGION do
Expand Down Expand Up @@ -1237,102 +1254,103 @@ end );
## <#/GAPDoc>
##

BIND_GLOBAL( "DeclareAttribute", function ( arg )
local name, gvar, req, reqs, filter, setter, tester,
attr, nname, mutflag, flags, rank;
BIND_GLOBAL( "ConvertToAttribute",
function(name, op, filter, rank, mutable)
local req, reqs, flags, nname;
# `op' is not an attribute (tester) and not a property (tester),
# or `op' is a filter; in any case, `op' is not an attribute.

name:= arg[1];
# if `op' has no one argument declarations we can turn it into
# an attribute
req := GET_OPER_FLAGS(op);
for reqs in req do
if LENGTH(reqs) = 1 then
Error( "operation `", name, "' has been declared as a one ",
"argument Operation and cannot also be an Attribute");
fi;
od;

if ISB_GVAR( name ) then
flags := FLAGS_FILTER(filter);
STORE_OPER_FLAGS( op, [ FLAGS_FILTER( filter ) ] );

atomic FILTER_REGION do
# The variable exists already.
gvar:= VALUE_GLOBAL( name );
# kernel magic for the conversion
if mutable then
OPER_TO_MUTABLE_ATTRIBUTE(op);
else
OPER_TO_ATTRIBUTE(op);
fi;

# Check that the variable is in fact bound to an operation.
if not IS_OPERATION( gvar ) then
Error( "variable `", name, "' is not bound to an operation" );
fi;
OPER_SetupAttribute(op, flags, mutable, filter, rank, name);

# The attribute has already been declared.
# If it was not created as an attribute
# then we may be able to convert it
if FLAG2_FILTER( gvar ) = 0 or IS_ELEMENTARY_FILTER(gvar) then
# and make the remaining assignments
nname:= "Set"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, SETTER_FILTER(op) );
nname:= "Has"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, TESTER_FILTER(op) );
end);

# `gvar' is not an attribute (tester) and not a property (tester),
# or `gvar' is a filter;
# in any case, `gvar' is not an attribute.

# if `gvar' has no one argument declarations we can turn it into
# an attribute
req := GET_OPER_FLAGS(gvar);
for reqs in req do
if LENGTH(reqs) = 1 then
Error( "operation `", name, "' has been declared as a one ",
"argument Operation and cannot also be an Attribute");
fi;
od;
mutflag := LEN_LIST(arg) = 3 and arg[3] = "mutable";

# add the new set of requirements
filter:= arg[2];
if not IS_OPERATION( filter ) then
Error( "<filter> must be an operation" );
fi;

flags := FLAGS_FILTER(filter);
STORE_OPER_FLAGS( gvar, [ FLAGS_FILTER( filter ) ] );

# kernel magic for the conversion
if mutflag then
OPER_TO_MUTABLE_ATTRIBUTE(gvar);
else
OPER_TO_ATTRIBUTE(gvar);
fi;

# now we have to adjust the data structures

if LEN_LIST(arg) = 3 and IS_INT(arg[3]) then
rank := arg[3];
else
rank := 1;
fi;
OPER_SetupAttribute(gvar, flags, mutflag, filter, rank, name);
# and make the remaining assignments
nname:= "Set"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, SETTER_FILTER(gvar) );
nname:= "Has"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, TESTER_FILTER(gvar) );

return;


fi;
BIND_GLOBAL( "DeclareAttribute", function ( name, filter, args... )
local gvar, req, reqs, setter, tester,
attr, nname, mutflag, flags, rank;

# Add the new requirements.
filter:= arg[2];
if not IS_OPERATION( filter ) then
Error( "<filter> must be an operation" );
fi;
STORE_OPER_FLAGS( gvar, [ FLAGS_FILTER( filter ) ] );
if not IS_STRING( name ) then
Error( "<name> must be a string");
fi;

# also set the extended range for the setter
req := GET_OPER_FLAGS( Setter(gvar) );
STORE_OPER_FLAGS( Setter(gvar), [ FLAGS_FILTER( filter), req[1][2] ] );
if not IsFilter( filter ) then
Error( "<filter> must be a filter" );
fi;

od;
rank := 1;
mutflag := false;
if LEN_LIST(args) = 0 then
# this is fine, but does nothing
elif LEN_LIST(args) = 1 and args[1] in [ "mutable", true, false ] then
mutflag := args[1] in [ "mutable", true];
elif LEN_LIST(args) = 1 and IS_INT(args[1]) then
rank := args[1];
elif LEN_LIST(args) = 2
and args[1] in [ "mutable", true, false ]
and IS_INT(args[2]) then
mutflag := args[1] in [ "mutable", true ];
rank := args[2];
else
Error("Usage: DeclareAttribute( <name>, <filter>[, <mutable>][, <rank>] )");
fi;

# The attribute is new.
attr:= CALL_FUNC_LIST( NewAttribute, arg );
BIND_GLOBAL( name, attr );

# and make the remaining assignments
nname:= "Set"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, SETTER_FILTER(attr) );
nname:= "Has"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, TESTER_FILTER( attr ) );
if ISB_GVAR( name ) then
atomic FILTER_REGION do
# The variable exists already.
gvar := VALUE_GLOBAL( name );

# Check that the variable is in fact bound to an operation.
if not IS_OPERATION( gvar ) then
Error( "variable `", name, "' is not bound to an operation" );
fi;

# The attribute has already been declared.
# If it was not created as an attribute
# then we may be able to convert it
if FLAG2_FILTER( gvar ) = 0 or IS_ELEMENTARY_FILTER(gvar) then
ConvertToAttribute(name, gvar, filter, rank, mutflag);
else
STORE_OPER_FLAGS( gvar, [ FLAGS_FILTER( filter ) ] );

# also set the extended range for the setter
req := GET_OPER_FLAGS( Setter(gvar) );
STORE_OPER_FLAGS( Setter(gvar), [ FLAGS_FILTER( filter), req[1][2] ] );
fi;
od;
else
# The attribute is new.
attr := NewAttribute(name, filter, mutflag, rank);
Copy link
Member

@fingolfin fingolfin Jan 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meta-issue (i.e. not something I expect this PR to address): I think this code (and that of other New/Declare functions) has a race condition in HPC-GAP: If e.g. DeclareAttribute is called from two threads simultaneously with identical arguments, then it could happen that bother check ISB_GVAR and decide the var is not bound, then go on to call NewAttribute, and try to BIND_GLOBAL at the same time. At this point, one of the two will run into an error.

We probably should open an issue about this to not forget ...

BIND_GLOBAL( name, attr );

# and make the remaining assignments
nname := "Set"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, SETTER_FILTER(attr) );
nname := "Has"; APPEND_LIST_INTR( nname, name );
BIND_GLOBAL( nname, TESTER_FILTER( attr ) );
fi;
end );

Expand Down
Loading