-
Notifications
You must be signed in to change notification settings - Fork 162
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
Fix missing syntax warning for using undefined gvar #2595
Fix missing syntax warning for using undefined gvar #2595
Conversation
Good catch. |
This needs a rebase. |
c58b257
to
6b97397
Compare
Codecov Report
@@ Coverage Diff @@
## master #2595 +/- ##
==========================================
+ Coverage 76.09% 76.1% +<.01%
==========================================
Files 480 480
Lines 240982 240984 +2
==========================================
+ Hits 183386 183389 +3
+ Misses 57596 57595 -1
|
The relevant changes are in transgrp 2.0.3. So once that is in the distribution, I can update this PR to change GAP to require transgrp >= 2.0.3, after which we could in principle merge it. |
2.0.3 has not been picked up because of hulpke/transgrp#34. Now waiting for version 2.0.4 from @hulpke. |
@alex-konovalov |
@hulpke ah, thanks for letting know. Check for package updates is scheduled for 9am daily from Monday till Friday, so I had no chance to find it out yet. |
Let me know when that new transpkg is picked up, then I can rebase this PR to trigger a rebuild and fresh tests. That said, from my POV, there really is no need to rush this PR into 4.10; it could easily wait till 4.11. |
You want to ask me to let you know when it is picked up and appears in packages-master.tar.gz archive. The former already happened, the latter - not yet. I will let you know. The chance of getting it for 4.10 is 80%. |
@fingolfin new TransGrp now approved and appears in packages-master.tar.gz and packages-stable-4.9.tar.gz archives. I will restart travis test to see whether it will now pass. |
@fingolfin please rebase, otherwise tests fail due to package build errors, addressed recently. |
6b97397
to
3d25ce2
Compare
Done |
GitHub did not receive the CI completion event, so I rerun one of the Travis jobs, and now GitHub noticed that all tests passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fingolfin.
@sebasguts since #1800 was created by you, are you happy with this PR?
I am going to rebase this once more before merge, just to make sure no weird conflict with master has crept in (I really wish we had something like https://bors.tech to take care of that; that one unfortunately does not support our rebase work flow, although they plan to add it eventually). |
3d25ce2
to
5ebf775
Compare
We normally issue a warning when code is parsed that uses a variable identifier which has not yet been defined, which GAP then resolves as the name of a gvar. However, we suppress this warning if it is for the assignment to a global variable. But in some rare cases, this suppressed too much: if the very next line after such an assignment wasn't an assignment itself, and accessed the gvar, we did not print a warning.
5ebf775
to
c216861
Compare
We normally issue a warning when code is parsed that uses a variable
identifier which has not yet been defined, which GAP then resolves as the
name of a gvar.
However, we suppress this warning if it is for the assignment to a global
variable.
But in some rare cases, this suppressed too much: if the very next line after
such an assignment wasn't an assignment itself, and accessed the gvar, we did
not print a warning.
This addresses part of issue #1800, though it does not implement the feature "requested" by @sebasguts there.
It also shows up one "problem" (?) in transgrp:
Note that
TRANSAVAILABLE
actually gets defined before the offending line; but since it is wrapped in an atomic (and hence coding is enabled), the warning gets triggered. Simple fix would be to initTRANSAVAILABLE
outside of that atomic; or even get rid of the atomic (it's not clear to me what it is for). I'll try to look into this and prepare a patch for transgrp. In the meantime, we should of course not merge this.