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

Suppress 'Unbound global variable' warning in IsBound #1334

Merged
merged 2 commits into from
May 16, 2017

Conversation

ChrisJefferson
Copy link
Contributor

At the moment, IsBound(HPCGAP) produces an Syntax warning: Unbound global variable warning when used in a function. This is a long-standing issue, but this easy change fixes it.

@codecov
Copy link

codecov bot commented May 12, 2017

Codecov Report

Merging #1334 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1334      +/-   ##
==========================================
- Coverage   61.53%   61.48%   -0.05%     
==========================================
  Files        1038     1049      +11     
  Lines      358256   365574    +7318     
  Branches    14289    15153     +864     
==========================================
+ Hits       220462   224788    +4326     
- Misses     134142   136764    +2622     
- Partials     3652     4022     +370
Impacted Files Coverage Δ
src/read.c 81.18% <100%> (+0.1%) ⬆️
src/hpc/threadapi.c 30.84% <0%> (-0.2%) ⬇️
hpcgap/src/stats.c 67.58% <0%> (ø) ⬆️
lib/kbsemi.gi 84.01% <0%> (ø) ⬆️
lib/process.gi 0% <0%> (ø) ⬆️
lib/pquot.gi 81% <0%> (ø) ⬆️
lib/options.gi 83.33% <0%> (ø) ⬆️
src/listoper.c 73.55% <0%> (ø) ⬆️
grp/imf.grp 100% <0%> (ø) ⬆️
src/hpc/traverse.c 79.84% <0%> (ø) ⬆️
... and 25 more

src/read.c Outdated
@@ -502,6 +502,7 @@ void ReadCallVarAss (
WarnOnUnboundGlobalsRNam = RNamName("WarnOnUnboundGlobals");

if ( type == 'g'
&& mode != 'i'
&& STATE(CountNams) != 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe document what these magic values mean.

src/read.c Outdated
&& (GAPInfo == 0 || !IS_REC(GAPInfo) || !ISB_REC(GAPInfo,WarnOnUnboundGlobalsRNam) ||
ELM_REC(GAPInfo,WarnOnUnboundGlobalsRNam) != False )
&& ! SyCompilePlease )
if ( type == 'g' /* Reading a global variable */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using single-line comments // Reading ... for these new comments? Now that we agreed C99 is fine... :-). Not that this is important in any way

src/read.c Outdated
&& VAL_GVAR(var) == 0 /* Not an existing global var */
&& ExprGVar(var) == 0 /* Or an auto var */
&& ! STATE(IntrIgnoring) /* Not currently ignoring parsed code */
&& ! GlobalComesFromEnclosingForLoop(var) /* ? */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the /* ? */ ? Doesn't seem to be helpful either way. Perhaps: "Don't warn again for index variable of an enclosing for loop"
An example where this matters:

f:=function()
  for j in [1..4] do
    Print(j,"\n"); od;
end;

If j is not defined, GAP only warns for the first occurrence of j. With the GlobalComesFromEnclosingForLoop call removed, it warns on both. Which kind of makes sense, because by the time you execute the second line, j will no longer be undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /* ? */ was because I couldn't figure out what that was for. Probably should have cleaned that up before submitting. Thanks for figuring it out.

@ChrisJefferson
Copy link
Contributor Author

Tweaked the warning message, as requested by @fingolfin

@fingolfin fingolfin merged commit 1f08efa into gap-system:master May 16, 2017
@ChrisJefferson ChrisJefferson deleted the isbound_unbound_var branch May 16, 2017 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants