Refactor DeclareAttribute and NewAttribute #2091

markuspf · 2018-01-15T17:01:34Z

This PR rearranges the argument parsing code for DeclareAttribute and NewAttribute, and addresses the issue that the rank argument would be ignored if "mutable" was given.

It would be good if @stevelinton could comment whether this might have been intentional (not being able to rank a tester for a mutable attribute).

Most of oper.g (DeclareProperty, DeclareConstructor,...) could do with a similar treatment.

(This PR includes some adjustments made responding to comments on #2080 by @fingolfin)

stevelinton · 2018-01-16T11:20:52Z

I can't think of any reason for this being intentional. I can quite imagine wanting to rank up something like HasMutableStabChain. It makes less sense for something like HasKnownSylowSubgroups (or whatever it's called) because the filter doesn't tell you WHICH Sylow subgroups it has.

codecov · 2018-01-16T14:06:10Z

Codecov Report

Merging #2091 into master will increase coverage by 0.01%.
The diff coverage is 92.15%.

@@            Coverage Diff             @@
##           master    #2091      +/-   ##
==========================================
+ Coverage   69.22%   69.23%   +0.01%     
==========================================
  Files         489      489              
  Lines      256237   256274      +37     
==========================================
+ Hits       177369   177436      +67     
+ Misses      78868    78838      -30

Impacted Files	Coverage Δ
lib/oper.g	`76.17% <92.1%> (+1.52%)`	⬆️
hpcgap/lib/oper.g	`73.48% <92.2%> (+1.53%)`	⬆️
lib/filter.g	`94.05% <0%> (-0.06%)`	⬇️
src/hpc/threadapi.c	`38.56% <0%> (ø)`	⬆️
hpcgap/lib/filter.g	`94.64% <0%> (ø)`	⬆️
src/opers.c	`81.46% <0%> (+0.05%)`	⬆️
src/funcs.c	`78.44% <0%> (+0.13%)`	⬆️
src/hpc/traverse.c	`84.1% <0%> (+0.38%)`	⬆️
src/calls.c	`56.21% <0%> (+0.51%)`	⬆️
lib/test.gi	`65.23% <0%> (+3.09%)`	⬆️
... and 1 more

markuspf · 2018-01-17T23:07:22Z

I am also getting the urge to factor the argument checking code into a separate function for DeclareAttribute and NewAttribute, since it is doing the same checks, and it was already a pain to keep the four copies in sync.

ChrisJefferson

This looks good, except for one issue I mention. This kind of code is always a bit horrible, so the ultimate test will be to test all packages/library code, and check nothing breaks by doing anything too crazy/unexpected.

ChrisJefferson · 2018-01-20T17:02:01Z

hpcgap/lib/oper.g

+    elif LEN_LIST(args) = 2 and args[1] in [ "mutable", true ] and IS_INT(args[2]) then
+        mutflag := true;
+        rank := args[2];
+    elif LEN_LIST(args) > 2 then


I think the logic is wrong here.. I think line 1202 should just be "else", otherwise if you get one or two arguments that don't match one of the earlier if/elifs, we silently accept them and do nothing?

ChrisJefferson · 2018-01-20T17:03:11Z

hpcgap/lib/oper.g

+    elif LEN_LIST(args) = 2 and args[1] in [ "mutable", true ] and IS_INT(args[2]) then
+        mutflag := true;
+        rank := args[2];
+    elif LEN_LIST(args) > 2 then


Same problem as mentioned above

ChrisJefferson · 2018-01-20T17:03:38Z

lib/oper.g

+    elif LEN_LIST(args) = 2 and args[1] in [ "mutable", true ] and IS_INT(args[2]) then
+        mutflag := true;
+        rank := args[2];
+    elif LEN_LIST(args) > 2 then


same issue as above

ChrisJefferson · 2018-01-20T17:03:54Z

lib/oper.g

+    elif LEN_LIST(args) = 2 and args[1] in [ "mutable", true ] and IS_INT(args[2]) then
+        mutflag := true;
+        rank := args[2];
+    elif LEN_LIST(args) > 2 then


same issue as above

fingolfin

I'll look some more, but here a few quick comments

fingolfin · 2018-01-20T10:48:44Z

hpcgap/lib/oper.g

+        mutflag := true;
+        rank := args[2];
+    elif LEN_LIST(args) > 2 then
+        Error("Usage: NewAttribute( <name>, <filter>, [<mutable>, <rank>] )");


This should match the usage string at the start of the documentation comment, i.e. <name>, <filter>[, <mutable>][, <rank>]

fingolfin · 2018-01-20T10:58:16Z

hpcgap/lib/oper.g


    if not IS_OPERATION( filter ) then
-      Error( "<filter> must be an operation" );
+        Error( "<filter> must be an operation" );


Perhaps also check that name is a string?

fingolfin · 2018-01-20T11:07:10Z

hpcgap/lib/oper.g

+        mutflag := true;
+        rank := args[2];
+    elif LEN_LIST(args) > 2 then
+        Error("Usage: DeclareAttribute( <name>, <filter>, [<mutable>, <rank>] )");


Adjust this to match the doc comment -- and perhaps both should be adjusted to match that of NewAttribute? I.e. <name>, <filter>[, <mutable>][, <rank>] ?

markuspf · 2018-01-22T13:26:02Z

Argument checking code rewritten again.

I also added tests to convince myself I cover all cases.

fingolfin

I think this is essentially good now. Sure, one could perhaps improve some of the doc comments, but I can easily do that in a future PR myself.

Unfortunately, this now conflicts with recent changes on master, but I think a rebase will go through mostly clean -- if not, I am happy to help if needed.

Also, the HPC-GAP tests now fail?

In any case, I'd like to get this merged ASAP so that I can look into syncing lib/oper.g and hpcgap/lib/oper.g.

fingolfin · 2018-01-20T17:54:49Z

hpcgap/lib/oper.g

+##  <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


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

fingolfin · 2018-01-20T18:07:03Z

hpcgap/lib/oper.g

-##  mutable.)
+##  For the optional third and fourth arguments, there are the following
+##  possibilities.
+##  <List>


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...

fingolfin · 2018-01-20T18:07:35Z

hpcgap/lib/oper.g

+        # 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 gvar in FILTERS then


That line now conflicts with my PR #2096. But I can easily fix that in that PR once this one here has been merged, no problem.

fingolfin · 2018-01-20T18:08:26Z

hpcgap/lib/oper.g

+      od;
+    else
+        # The attribute is new.
+        attr := NewAttribute(name, filter, mutflag, rank);


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 ...

This is the result of trying to fix a bug in DeclareAttribute where if the user called it with the same name but different filters, the attributes would be setup correctly, but the second would not be added to the list ATTRIBUTES. The argument parsing made it impossible to give "mutable" and a rank for the attribute: If one gave "mutable", the rank would be ignored.

olexandr-konovalov · 2018-01-29T20:15:01Z

Added to release notes for GAP 4.10 at https://github.com/gap-system/gap/wiki/GAP-4.10-release-notes

markuspf force-pushed the refactor-declare-attribute branch from c29f290 to 742f5ae Compare January 16, 2018 10:35

markuspf force-pushed the refactor-declare-attribute branch from 742f5ae to f18b162 Compare January 16, 2018 14:05

markuspf requested review from fingolfin, ChrisJefferson and frankluebeck January 19, 2018 13:09

ChrisJefferson approved these changes Jan 20, 2018

View reviewed changes

fingolfin reviewed Jan 20, 2018

View reviewed changes

markuspf force-pushed the refactor-declare-attribute branch 2 times, most recently from 2ce8f3a to e86dab0 Compare January 22, 2018 13:22

markuspf force-pushed the refactor-declare-attribute branch from e86dab0 to b5ec9b2 Compare January 22, 2018 15:25

fingolfin approved these changes Jan 22, 2018

View reviewed changes

markuspf and others added 5 commits January 22, 2018 17:06

Sync hpcgap/lib/oper.g

aeda08f

Update documentation of DeclareAttribute

fb4f8c0

Sync documentation of DeclareAttribute to HPC-GAP

141349e

Add tests for the argument parsing code of NewAttribute/DeclareAttribute

60ad35d

markuspf force-pushed the refactor-declare-attribute branch from b5ec9b2 to 60ad35d Compare January 22, 2018 17:09

fingolfin merged commit d321602 into gap-system:master Jan 23, 2018

olexandr-konovalov added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: added PRs introducing changes that have since been mentioned in the release notes labels Jan 29, 2018

olexandr-konovalov added this to the GAP 4.10.0 milestone Jan 29, 2018

olexandr-konovalov changed the title ~~Refactor declare attribute~~ Refactor DeclareAttribute and NewAttribute Jan 29, 2018

olexandr-konovalov added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Jan 29, 2018

markuspf deleted the refactor-declare-attribute branch February 19, 2018 15:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor DeclareAttribute and NewAttribute #2091

Refactor DeclareAttribute and NewAttribute #2091

markuspf commented Jan 15, 2018 •

edited

Loading

stevelinton commented Jan 16, 2018

codecov bot commented Jan 16, 2018 •

edited

Loading

markuspf commented Jan 17, 2018

ChrisJefferson left a comment

ChrisJefferson Jan 20, 2018

ChrisJefferson Jan 20, 2018

ChrisJefferson Jan 20, 2018

ChrisJefferson Jan 20, 2018

fingolfin left a comment

fingolfin Jan 20, 2018

fingolfin Jan 20, 2018

fingolfin Jan 20, 2018

markuspf commented Jan 22, 2018

fingolfin left a comment

fingolfin Jan 20, 2018

fingolfin Jan 20, 2018

fingolfin Jan 20, 2018

fingolfin Jan 20, 2018

olexandr-konovalov commented Jan 29, 2018 •

edited

Loading

Refactor DeclareAttribute and NewAttribute #2091

Refactor DeclareAttribute and NewAttribute #2091

Conversation

markuspf commented Jan 15, 2018 • edited Loading

stevelinton commented Jan 16, 2018

codecov bot commented Jan 16, 2018 • edited Loading

Codecov Report

markuspf commented Jan 17, 2018

ChrisJefferson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf commented Jan 22, 2018

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olexandr-konovalov commented Jan 29, 2018 • edited Loading

markuspf commented Jan 15, 2018 •

edited

Loading

codecov bot commented Jan 16, 2018 •

edited

Loading

olexandr-konovalov commented Jan 29, 2018 •

edited

Loading