Skip to content
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

Merged
merged 1 commit into from
Sep 21, 2018

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jun 29, 2018

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:

Syntax warning: Unbound global variable in GAPROOT/pkg/transgrp/lib/trans.grp:240
	  TRANSAVAILABLE[tradeg]:=true;
	                ^

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

@ChrisJefferson
Copy link
Contributor

Good catch.

@markuspf
Copy link
Member

This needs a rebase.

@codecov
Copy link

codecov bot commented Aug 21, 2018

Codecov Report

Merging #2595 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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
Impacted Files Coverage Δ
src/read.c 96.57% <100%> (+0.09%) ⬆️

@fingolfin
Copy link
Member Author

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.

@markuspf markuspf added kind: bug Issues describing general bugs, and PRs fixing them release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 22, 2018
@olexandr-konovalov
Copy link
Member

2.0.3 has not been picked up because of hulpke/transgrp#34. Now waiting for version 2.0.4 from @hulpke.

@olexandr-konovalov olexandr-konovalov added this to the GAP 4.10.0 milestone Sep 2, 2018
@hulpke
Copy link
Contributor

hulpke commented Sep 2, 2018

@alex-konovalov
2.0.4 has been in place since Friday.

@olexandr-konovalov
Copy link
Member

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

@fingolfin
Copy link
Member Author

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.

@olexandr-konovalov
Copy link
Member

@fingolfin

Let me know when that new transpkg is picked up, then I can rebase this PR to trigger a rebuild and fresh tests.

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

@olexandr-konovalov
Copy link
Member

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

@olexandr-konovalov
Copy link
Member

@fingolfin please rebase, otherwise tests fail due to package build errors, addressed recently.

@fingolfin
Copy link
Member Author

Done

@fingolfin
Copy link
Member Author

GitHub did not receive the CI completion event, so I rerun one of the Travis jobs, and now GitHub noticed that all tests passed.

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a 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?

@fingolfin
Copy link
Member Author

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

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.
@fingolfin fingolfin merged commit f304c01 into gap-system:master Sep 21, 2018
@fingolfin fingolfin deleted the mh/stricter-CurrLHSGVar branch September 21, 2018 07:11
@wucas wucas added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants