Refactor declare attribute #2080
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fingolfin
reviewed
Jan 12, 2018
hpcgap/lib/oper.g
Outdated
| rank := args[2]; | ||
| else | ||
| rank := 1; | ||
| fi; |
Member
There was a problem hiding this comment.
Perhaps we should also reject invalid arguments? E.g.:
mutflag:=false;
rank:= 1;
if LEN_LIST(args) = 1 and args[1] in [ true, false ] then
mutflag := args[1];
elif LEN_LIST(args) = 1 and IS_INT(args[1]) then
rank := args[1];
elif LEN_LIST(args) = 2 and args[1] in [true, false] and IS_INT(args[2]) then
mutflag := args[1];
rank := args[2];
else
Error("Usage: NewAttribute(...)");
fi;
hpcgap/lib/oper.g
Outdated
|
|
||
| atomic FILTER_REGION do | ||
| OPER_SetupAttribute(getter, flags, mutflag, filter, rank, name); | ||
| OPER_SetupAttribute(getter, flags, mutflag, filter, rank, name); |
Member
There was a problem hiding this comment.
I deliberately removed indents like that to reduce spurious diffs without whitespace suppression. Granted, in this particular case it matters little, but for consistency, I'd like to keep it as it was.
markuspf
force-pushed
the
refactor-declare-attribute
branch
from
8a5874f to
936f7d8
Compare
Codecov Report
@@ Coverage Diff @@
## master #2080 +/- ##
==========================================
- Coverage 67.84% 61.31% -6.54%
==========================================
Files 489 493 +4
Lines 255602 260657 +5055
==========================================
- Hits 173420 159811 -13609
- Misses 82182 100846 +18664
|
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.
markuspf
force-pushed
the
refactor-declare-attribute
branch
from
936f7d8 to
a8bf825
Compare
olexandr-konovalov
pushed a commit
to gap-system/gap-distribution
that referenced
this pull request
Jan 25, 2018
Loading them causes error as uncovered by refactoring DeclareAttribute (gap-system/gap#2080)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the first part of what used to be #2078.
It rearranges the argument parsing code for
DeclareAttributeandNewAttribute, 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).