Make the assertion level (which controls Assert statements) thread local when using HPC-GAP
#3822
Conversation
fingolfin
added
topic: HPC-GAP
fingolfin
force-pushed
the
mh/CurrentAssertionLevel
branch
2 times, most recently
from
e24b3a9
to
50ad246
Compare
|
One first comment, That INT_INTOBJ -> Int_ObjInt change I like, but I'd like it in a separate commit (either in this PR, or seperately). Last time I edited this code I left the GAP-level CurrentAssertionLevel, because I was worried someone might be using it. Any code which was using it will now silently just change a GAP global variable without doing anything, which slightly worries me. Could we instead just make the variable thread-local at the GAP level? |
fingolfin
force-pushed
the
mh/CurrentAssertionLevel
branch
from
50ad246
to
394e3e6
Compare
|
@ChrisJefferson split into two commits (good idea, thanks). Making |
|
|
||
| static Obj FuncAssertionLevel(Obj self) | ||
| { | ||
| return INTOBJ_INT(STATE(CurrentAssertionLevel)); |
There was a problem hiding this comment.
BTW, placing these into gap.c was somewhat arbitrary. If somebody has a preference for another location, let me know.
| @@ -12,6 +12,8 @@ gap> f('x'); | |||
| Error, Range: <last> must be a small integer (not a character) | |||
|
|
|||
| # test Assert with two arguments | |||
| gap> function() Assert(fail, 0); end(); | |||
| Error, Assert: <lev> must be a small integer (not the value 'fail') | |||
There was a problem hiding this comment.
For the record, I am using <lev> here as that matches what the documentation for Assert calls that argument (that's my general rule for the name of the arguments in error messages). I am fine with changing it here, e.g. to level, but IMHO it then should be changed in the documentation as well.
Fix a potential issue in gac where `INT_INTOBJ` was called on inputs without validation; this could lead to unexpected behavior. We now use `Int_ObjInt` instead, which raises an error if the input is not a small integer.
... by moving CurrentAssertionLevel to the kernel, into the GAPState. While at it, also validate the level argument given to Assert(), and error out if it is not a non-negative small integer. While no code should have ever directly accessed CurrentAssertionLevel, there is a minimal chance that some legacy code somewhere does this. To make sure it suddenly runs with an incorrect assertion level without anybody noticing, we keep defining CurrentAssertionLevel, but as a constant bound to `false`. This way, any code trying to modify it, or to compare it against an integer, will run into an error.
fingolfin
force-pushed
the
mh/CurrentAssertionLevel
branch
from
394e3e6
to
c17e62f
Compare
ThomasBreuer
added
release notes: added
... by moving
CurrentAssertionLevelto the kernel, into the GAPState.While at it, also validate the level argument given to Assert(), and error out if it is not a non-negative small integer.
Also fix a potential issue in gac where INT_INTOBJ was called on inputs without validation; we now use Int_ObjInt instead, which produces an error if the input is not a small integer.
Resolves #3821