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

Add IsAutoGlobal for testing whether a variable was declared using DeclareAutoreadableVariables #3076

Merged
merged 3 commits into from
Dec 5, 2018

Conversation

ChrisJefferson
Copy link
Contributor

At the moment there is no way to find if a variable is an "auto" (which means a function will be run when you access it). This is a problem in the (rare) cases where one wants to iterate over every variable in GAP, as just reading the value of an auto variable can cause lots of code to run.

Fixes (hopefully) a bug in the SYNTAX_TREE test

## <Func Name="IsAutoGlobal" Arg='name'/>
##
## <Description>
## IsAutoGlobal ( <A>name</A> ) returns true if there is 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.

I do realize you just copied the style used elsewhere in this file, but this inconsistent with what we do elsewhere. The following three alternatives come to mind (and we sadly use all of them in some place or another; perhaps we ought to come up with some style guidelines for our docs, and try to enforce them globally?)

Suggested change
## IsAutoGlobal ( <A>name</A> ) returns true if there is a global variable
## returns true if there is a global variable

or

Suggested change
## IsAutoGlobal ( <A>name</A> ) returns true if there is a global variable
## Returns true if there is a global variable

or

Suggested change
## IsAutoGlobal ( <A>name</A> ) returns true if there is a global variable
## <Ref Func="IsAutoGlobal"/> returns true if there is a global variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not opposed to these changes, but I would also prefer if we did try to keep the various *Global functions consistent with each other, so change them all, or none of them.

Copy link
Member

Choose a reason for hiding this comment

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

OK, then let's do those with another PR someday in the future.

lib/global.gi Show resolved Hide resolved
lib/global.gi Show resolved Hide resolved
src/gvars.c Outdated Show resolved Hide resolved
src/gvars.c Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 3, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3076      +/-   ##
==========================================
+ Coverage   83.85%   83.85%   +<.01%     
==========================================
  Files         688      688              
  Lines      343238   343230       -8     
==========================================
+ Hits       287818   287822       +4     
+ Misses      55420    55408      -12
Impacted Files Coverage Δ
lib/global.gi 53.67% <100%> (+1.9%) ⬆️
src/gvars.c 86.15% <100%> (+0.47%) ⬆️
lib/global.gd 100% <100%> (ø) ⬆️
lib/record.gi 75.94% <0%> (-2.63%) ⬇️
lib/files.gd 74.28% <0%> (-0.37%) ⬇️
lib/record.gd 100% <0%> (ø) ⬆️
lib/string.gd 100% <0%> (ø) ⬆️
lib/string.gi 77.88% <0%> (+0.43%) ⬆️
lib/files.gi 68.04% <0%> (+1.92%) ⬆️

@ChrisJefferson
Copy link
Contributor Author

I've fixed the bug and added a test for hpcgap (thanks @fingolfin). I'd prefer to not change the docs here, but make larger consistent changes in another PR.

@markuspf markuspf merged commit 67fb341 into gap-system:master Dec 5, 2018
@ChrisJefferson ChrisJefferson deleted the autogvar branch January 20, 2019 13:21
@PaulaHaehndel PaulaHaehndel added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Apr 15, 2019
@fingolfin fingolfin changed the title Add IsAutoGlobal Add IsAutoGlobal for testing whether a variable was declared using DeclareAutoreadableVariables May 3, 2019
@fingolfin fingolfin 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 21, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

5 participants